Re: [PATCH v2 11/23] staging: media: tegra-video: csi: add a check to tegra_channel_get_remote_csi_subdev
From: Luca Ceresoli <luca.ceresoli@bootlin.com>
Date: 2025-09-16 16:04:40
Also in:
dri-devel, linux-clk, linux-media, linux-staging, linux-tegra, lkml
Hello Svyatoslav, On Sat, 6 Sep 2025 16:53:32 +0300 Svyatoslav Ryhel [off-list ref] 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?
quoted hunk ↗ jump to hunk
--- 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