Hello Svyatoslav,
On Sat, 6 Sep 2025 16:53:32 +0300
Svyatoslav Ryhel <clamo...@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
