ср, 17 вер. 2025 р. о 10:25 Luca Ceresoli [off-list ref] пише:

Hello Svyatoslav,

On Tue, 16 Sep 2025 19:24:52 +0300
Svyatoslav Ryhel [off-list ref] wrote:

quoted

вт, 16 вер. 2025 р. о 19:04 Luca Ceresoli [off-list ref] пише:

quoted

Hello Svyatoslav,

On Sat,  6 Sep 2025 16:53:32 +0300
Svyatoslav Ryhel [off-list ref] wrote:

quoted

By default tegra_channel_get_remote_csi_subdev returns next device in pipe
assuming it is CSI but in case of Tegra20 and Tegra30 it can also be VIP
or even HOST. Lets check if returned device is actually CSI by comparing
subdevice operations.

This is just for extra safety, or is there a real case where the lack
of this check creates some issues in your use case?

quoted

--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c

@@ -445,6 +445,22 @@ static const struct v4l2_subdev_ops tegra_csi_ops = {
      .pad    = &tegra_csi_pad_ops,
 };

+struct v4l2_subdev *tegra_channel_get_remote_csi_subdev(struct tegra_vi_channel *chan)
+{
+     struct media_pad *pad;
+     struct v4l2_subdev *subdev;
+
+     pad = media_pad_remote_pad_first(&chan->pad);
+     if (!pad)
+             return NULL;
+
+     subdev = media_entity_to_v4l2_subdev(pad->entity);
+     if (!subdev)
+             return NULL;
+
+     return subdev->ops == &tegra_csi_ops ? subdev : NULL;
+}

I tested your series on a Tegra20 with a parallel camera, so using the
VIP for parallel input.

The added check on subdev->ops breaks probing the video device:

  tegra-vi 54080000.vi: failed to setup channel controls: -19
  tegra-vi 54080000.vi: failed to register channel 0 notifier: -19

This is because tegra20_chan_capture_kthread_start() is also calling
tegra_channel_get_remote_csi_subdev(), but when using VIP subdev->ops
points to tegra_vip_ops, not tegra_csi_ops.

Your assumption is wrong. 'tegra_channel_get_remote_csi_subdev' is
designed to get next device which is expected to be CSI, NOT VIP
(obviously, Tegra210 has no VIP). It seems that VIP implementation did
not take into account that CSI even exists.

IIRC it's rather the initial VI implementation was meant to be open to
supporting both VIP and CSI but some CSI assumptions sneaked in. Which
is somewhat unavoidable if only CSI could be tested, isn't it? So I had
to change some when adding VIP (trying hard myself to not break CSI and
T210).

It may be initial VI, that is not that important since my goal is not
blame anyone but to implement stuff I would like to see working. If my
words offended you, I am sorry for that.

quoted

 -19 errors are due to
tegra_vi_graph_notify_complete not able to get next media device in
the line. Correct approach would be to add similar helper for VIP and
check if next device is VIP.

I think it's almost correct.

tegra_channel_get_remote_csi_subdev() is called:
 * in vi.c, where it is expeted to return either a CSI or VIP subdev
 * in tegra210.c, which apparently supports CSI only
   (I don't know whether the hardware has parallel input)
 * in tegra20.c [added by patch 23 in this series] where only a CSI
   subdev is wanted

Based on that,  you're right that we need two functions, but they
should be:

 1. one to return the remote subdev, be it CSI or VIP
    a. perhaps called tegra_channel_get_remote_subdev()
    b. perhaps in vi.c
    c. not checking subdev->ops (or checking for csi||vip)
 2. one to return the remote subdev, only if it is CSI
    a. perhaps called tegra_channel_get_remote_csi_subdev()
    b. perhaps in csi.c
    c. checking subdev->ops == tegra_csi_ops

The function in mainline as of now complies with 2a, 1b, 1c, so it is a
hybrid.

In other words, what I propose is:

 * rename the current tegra_channel_get_remote_csi_subdev()
   to remove the "_csi" infix, so the name reflects what it does
   - optionally add the check for (csi||vip)
 * add tegra_channel_get_remote_csi_subdev() for where a CSI-only
   subdev is needed: that's exactly the function you are adding to csi.c
   in this patch

Does it look correct?

Yes, if this was your initial idea then I must have misunderstood you,
you are correct. We can agree that each VI source should have its own
get_remote_device function since VI configuration cannot be agnostic
in relation to the source. ATM since only CSI and VIP are supported we
can have only one for CSI and use VIP as default option, which is
fine. Meanwhile, since core VI configuration (vi.c) does not perform
any specific operations with VIs source, we can leave
tegra_channel_get_remote_csi_subdev structure as is, just call it smth
like tegra_channel_get_remote_bridge_subdev and use it in vi, it will
get any VIs source device in the pipe regardless of its type.

quoted

Since I have no devices with VIP support
I could not test this properly.

Of course, no problem. I can test it (but I cannot test CSI).

Good, CSI is not an issue, I am always checking if it remains functional.

quoted

I can add this in next iteration if
you are willing to test.

Yes, please do, thanks.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com