Re: [PATCH v2 11/23] staging: media: tegra-video: csi: add a check to tegra_channel_get_remote_csi_subdev

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

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