Message-ID: <20250917092506.311c314c@booty>
Date: Wed, 17 Sep 2025 09:25:06 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Svyatoslav Ryhel <clamor95@...il.com>
Cc: Thierry Reding <thierry.reding@...il.com>, Thierry Reding
 <treding@...dia.com>, Mikko Perttunen <mperttunen@...dia.com>, Jonathan
 Hunter <jonathanh@...dia.com>, Sowjanya Komatineni
 <skomatineni@...dia.com>, David Airlie <airlied@...il.com>, Simona Vetter
 <simona@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann
 <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Prashant Gaikwad
 <pgaikwad@...dia.com>, Michael Turquette <mturquette@...libre.com>, Stephen
 Boyd <sboyd@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Dmitry Osipenko
 <digetx@...il.com>, Jonas Schwöbel
 <jonasschwoebel@...oo.de>, Charan Pedumuru <charan.pedumuru@...il.com>,
 dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-media@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2 11/23] staging: media: tegra-video: csi: add a check
 to tegra_channel_get_remote_csi_subdev

Hello Svyatoslav,

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

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

Powered by blists - more mailing lists