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.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/staging/media/tegra-video/csi.c | 16 ++++++++++++++++
 drivers/staging/media/tegra-video/vi.c  | 12 ------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index 3d1d5e1615c2..c848e4ab51ac 100644
--- 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;
+}
+
 static int tegra_csi_channel_alloc(struct tegra_csi *csi,
 				   struct device_node *node,
 				   unsigned int port_num, unsigned int lanes,
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 90473729b546..2deb615547be 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -160,18 +160,6 @@ static void tegra_channel_buffer_queue(struct vb2_buffer *vb)
 	wake_up_interruptible(&chan->start_wait);
 }
 
-struct v4l2_subdev *
-tegra_channel_get_remote_csi_subdev(struct tegra_vi_channel *chan)
-{
-	struct media_pad *pad;
-
-	pad = media_pad_remote_pad_first(&chan->pad);
-	if (!pad)
-		return NULL;
-
-	return media_entity_to_v4l2_subdev(pad->entity);
-}
-
 /*
  * Walk up the chain until the initial source (e.g. image sensor)
  */
-- 
2.48.1

Hello Svyatoslav,

On Sat,  6 Sep 2025 16:53:32 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

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

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

Surely the "csi" infix in the function name here is misleading. At
quick glance I don't see a good reason for its presence however, as the
callers are not CSI-specific.

If such quick analysis is correct, instead of this diff we should:
 * not move the function out of vi.c
 * rename the function toremove the "_csi" infix
 * if a check is really needed (see comment above), maybe extend it:
   return (subdev->ops == &tegra_csi_ops ||
           subdev->ops == &tegra_vip_ops) ? subdev : NULL;

Let me know your thoughts.

Best regards,
Luca

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

вт, 16 вер. 2025 р. о 19:04 Luca Ceresoli <luca.ceresoli@bootlin.com> пише:
>
> Hello Svyatoslav,
>
> On Sat,  6 Sep 2025 16:53:32 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > 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?
>
> > --- 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.  -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. Since I have no devices with VIP support
I could not test this properly. I can add this in next iteration if
you are willing to test.

Best regards,
Svyatoslav R.

> Surely the "csi" infix in the function name here is misleading. At
> quick glance I don't see a good reason for its presence however, as the
> callers are not CSI-specific.
>
> If such quick analysis is correct, instead of this diff we should:
>  * not move the function out of vi.c
>  * rename the function toremove the "_csi" infix
>  * if a check is really needed (see comment above), maybe extend it:
>    return (subdev->ops == &tegra_csi_ops ||
>            subdev->ops == &tegra_vip_ops) ? subdev : NULL;
>
> Let me know your thoughts.
>
> Best regards,
> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Hello Svyatoslav,

On Tue, 16 Sep 2025 19:24:52 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> вт, 16 вер. 2025 р. о 19:04 Luca Ceresoli <luca.ceresoli@bootlin.com> пише:
> >
> > Hello Svyatoslav,
> >
> > On Sat,  6 Sep 2025 16:53:32 +0300
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >  
> > > 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?
> >  
> > > --- 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).

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

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

> 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

ср, 17 вер. 2025 р. о 10:25 Luca Ceresoli <luca.ceresoli@bootlin.com> пише:
>
> Hello Svyatoslav,
>
> On Tue, 16 Sep 2025 19:24:52 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > вт, 16 вер. 2025 р. о 19:04 Luca Ceresoli <luca.ceresoli@bootlin.com> пише:
> > >
> > > Hello Svyatoslav,
> > >
> > > On Sat,  6 Sep 2025 16:53:32 +0300
> > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > > 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?
> > >
> > > > --- 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.

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

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

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