Re: [PATCH v5 14/20] staging: media: tegra-video: move MIPI calibration calls from VI to CSI

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Luca,

I just encountered an error in this patch, so I have rejected the PR I made.

See below for the details:

On 07/04/2023 15:38, Luca Ceresoli wrote:
> The CSI module does not handle all the MIPI lane calibration procedure,
> leaving a small part of it to the VI module. In doing this,
> tegra_channel_enable_stream() (vi.c) manipulates the private data of the
> upstream subdev casting it to struct 'tegra_csi_channel', which will be
> wrong after introducing a VIP (parallel video input) channel.
> 
> This prevents adding support for the VIP module.  It also breaks the
> logical isolation between modules.
> 
> Since the lane calibration requirement does not exist in the parallel input
> module, moving the calibration function to a per-module op is not
> optimal. Instead move the calibration procedure in the CSI module, together
> with the rest of the calibration procedures. After this change,
> tegra_channel_enable_stream() just calls v4l2_subdev_call() to ask for a
> stream start/stop to the CSI module, which in turn knows all the
> CSI-specific details to implement it.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
> Reviewed-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> 
> ---
> 
> No changes in v5
> 
> Changed in v4:
>  - Added review tags
> 
> No changes in v3
> No changes in v2
> ---
>  drivers/staging/media/tegra-video/csi.c | 44 ++++++++++++++++++++
>  drivers/staging/media/tegra-video/vi.c  | 54 ++-----------------------
>  2 files changed, 48 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
> index 9a03d5ccdf3c..b93fc879ef3a 100644
> --- a/drivers/staging/media/tegra-video/csi.c
> +++ b/drivers/staging/media/tegra-video/csi.c
> @@ -328,12 +328,42 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
>  	}
>  
>  	csi_chan->pg_mode = chan->pg_mode;
> +
> +	/*
> +	 * Tegra CSI receiver can detect the first LP to HS transition.
> +	 * So, start the CSI stream-on prior to sensor stream-on and
> +	 * vice-versa for stream-off.
> +	 */
>  	ret = csi->ops->csi_start_streaming(csi_chan);
>  	if (ret < 0)
>  		goto finish_calibration;
>  
> +	if (csi_chan->mipi) {
> +		struct v4l2_subdev *src_subdev;
> +		/*
> +		 * TRM has incorrectly documented to wait for done status from
> +		 * calibration logic after CSI interface power on.
> +		 * As per the design, calibration results are latched and applied
> +		 * to the pads only when the link is in LP11 state which will happen
> +		 * during the sensor stream-on.
> +		 * CSI subdev stream-on triggers start of MIPI pads calibration.
> +		 * Wait for calibration to finish here after sensor subdev stream-on.
> +		 */
> +		src_subdev = tegra_channel_get_remote_source_subdev(chan);
> +		ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
> +		err = tegra_mipi_finish_calibration(csi_chan->mipi);
> +
> +		if (ret < 0 && ret != -ENOIOCTLCMD)
> +			goto disable_csi_stream;

If there was an error from s_stream, then tegra_mipi_finish_calibration is called
and it goes to disable_csi_stream...

> +
> +		if (err < 0)
> +			dev_warn(csi->dev, "MIPI calibration failed: %d\n", err);
> +	}
> +
>  	return 0;
>  
> +disable_csi_stream:
> +	csi->ops->csi_stop_streaming(csi_chan);
>  finish_calibration:
>  	if (csi_chan->mipi)
>  		tegra_mipi_finish_calibration(csi_chan->mipi);

...but here tegra_mipi_finish_calibration() is called again, leading to an unlock
imbalance.

This is the callstack:

[  109.894502] IMX274 5-001a: s_stream failed

[  109.900203] =====================================
[  109.904898] WARNING: bad unlock balance detected!
[  109.909594] 6.3.0-rc2-tegra #16 Not tainted
[  109.913774] -------------------------------------
[  109.918470] v4l2-ctl/2000 is trying to release lock (&mipi->lock) at:
[  109.924911] [<ffff80000866b828>] tegra_mipi_finish_calibration+0x84/0xb0
[  109.931621] but there are no more locks to release!
[  109.936489]
               other info that might help us debug this:
[  109.943004] 1 lock held by v4l2-ctl/2000:
[  109.947009]  #0: ffff000083bcf6a8 (&chan->video_lock){....}-{3:3}, at: __video_do_ioctl+0xdc/0x3c8
[  109.955987]
               stack backtrace:
[  109.960336] CPU: 2 PID: 2000 Comm: v4l2-ctl Not tainted 6.3.0-rc2-tegra #16
[  109.967290] Hardware name: NVIDIA Jetson TX1 Developer Kit (DT)
[  109.973200] Call trace:
[  109.975642]  dump_backtrace+0xa0/0xfc
[  109.979308]  show_stack+0x18/0x24
[  109.982622]  dump_stack_lvl+0x48/0x60
[  109.986285]  dump_stack+0x18/0x24
[  109.989598]  print_unlock_imbalance_bug+0x130/0x148
[  109.994472]  lock_release+0x1bc/0x248
[  109.998131]  __mutex_unlock_slowpath+0x48/0x2cc
[  110.002657]  mutex_unlock+0x20/0x2c
[  110.006141]  tegra_mipi_finish_calibration+0x84/0xb0
[  110.011102]  tegra_csi_s_stream+0x260/0x318
[  110.015286]  call_s_stream+0x80/0xcc
[  110.018857]  tegra_channel_set_stream+0x58/0xe4
[  110.023386]  tegra210_vi_start_streaming+0xb0/0x1c8
[  110.028262]  tegra_channel_start_streaming+0x54/0x134
[  110.033311]  vb2_start_streaming+0xbc/0x1b8
[  110.037491]  vb2_core_streamon+0x158/0x260
[  110.041582]  vb2_ioctl_streamon+0x4c/0x90
[  110.045589]  v4l_streamon+0x24/0x30
[  110.049076]  __video_do_ioctl+0x160/0x3c8
[  110.053082]  video_usercopy+0x1f0/0x658
[  110.056916]  video_ioctl2+0x18/0x28
[  110.060404]  v4l2_ioctl+0x40/0x60
[  110.063715]  __arm64_sys_ioctl+0xac/0xf0
[  110.067638]  invoke_syscall+0x48/0x114
[  110.071385]  el0_svc_common.constprop.0+0x44/0xec
[  110.076086]  do_el0_svc+0x38/0x98
[  110.079398]  el0_svc+0x2c/0x84
[  110.082454]  el0t_64_sync_handler+0xf4/0x120
[  110.086722]  el0t_64_sync+0x190/0x194

Regards,

	Hans

> @@ -352,10 +382,24 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
>  
>  static int tegra_csi_disable_stream(struct v4l2_subdev *subdev)
>  {
> +	struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
>  	struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
>  	struct tegra_csi *csi = csi_chan->csi;
>  	int err;
>  
> +	/*
> +	 * Stream-off subdevices in reverse order to stream-on.
> +	 * Remote source subdev in TPG mode is same as CSI subdev.
> +	 */
> +	if (csi_chan->mipi) {
> +		struct v4l2_subdev *src_subdev;
> +
> +		src_subdev = tegra_channel_get_remote_source_subdev(chan);
> +		err = v4l2_subdev_call(src_subdev, video, s_stream, false);
> +		if (err < 0 && err != -ENOIOCTLCMD)
> +			dev_err_probe(csi->dev, err, "source subdev stream off failed\n");
> +	}
> +
>  	csi->ops->csi_stop_streaming(csi_chan);
>  
>  	if (csi_chan->mipi) {
> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> index b88532d8d2c9..c76c2a404889 100644
> --- a/drivers/staging/media/tegra-video/vi.c
> +++ b/drivers/staging/media/tegra-video/vi.c
> @@ -197,49 +197,15 @@ tegra_channel_get_remote_source_subdev(struct tegra_vi_channel *chan)
>  
>  static int tegra_channel_enable_stream(struct tegra_vi_channel *chan)
>  {
> -	struct v4l2_subdev *csi_subdev, *src_subdev;
> -	struct tegra_csi_channel *csi_chan;
> -	int ret, err;
> +	struct v4l2_subdev *subdev;
> +	int ret;
>  
> -	/*
> -	 * Tegra CSI receiver can detect the first LP to HS transition.
> -	 * So, start the CSI stream-on prior to sensor stream-on and
> -	 * vice-versa for stream-off.
> -	 */
> -	csi_subdev = tegra_channel_get_remote_csi_subdev(chan);
> -	ret = v4l2_subdev_call(csi_subdev, video, s_stream, true);
> +	subdev = tegra_channel_get_remote_csi_subdev(chan);
> +	ret = v4l2_subdev_call(subdev, video, s_stream, true);
>  	if (ret < 0 && ret != -ENOIOCTLCMD)
>  		return ret;
>  
> -	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
> -		return 0;
> -
> -	csi_chan = v4l2_get_subdevdata(csi_subdev);
> -	/*
> -	 * TRM has incorrectly documented to wait for done status from
> -	 * calibration logic after CSI interface power on.
> -	 * As per the design, calibration results are latched and applied
> -	 * to the pads only when the link is in LP11 state which will happen
> -	 * during the sensor stream-on.
> -	 * CSI subdev stream-on triggers start of MIPI pads calibration.
> -	 * Wait for calibration to finish here after sensor subdev stream-on.
> -	 */
> -	src_subdev = tegra_channel_get_remote_source_subdev(chan);
> -	ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
> -	err = tegra_mipi_finish_calibration(csi_chan->mipi);
> -
> -	if (ret < 0 && ret != -ENOIOCTLCMD)
> -		goto err_disable_csi_stream;
> -
> -	if (err < 0)
> -		dev_warn(csi_chan->csi->dev,
> -			 "MIPI calibration failed: %d\n", err);
> -
>  	return 0;
> -
> -err_disable_csi_stream:
> -	v4l2_subdev_call(csi_subdev, video, s_stream, false);
> -	return ret;
>  }
>  
>  static int tegra_channel_disable_stream(struct tegra_vi_channel *chan)
> @@ -247,18 +213,6 @@ static int tegra_channel_disable_stream(struct tegra_vi_channel *chan)
>  	struct v4l2_subdev *subdev;
>  	int ret;
>  
> -	/*
> -	 * Stream-off subdevices in reverse order to stream-on.
> -	 * Remote source subdev in TPG mode is same as CSI subdev.
> -	 */
> -	subdev = tegra_channel_get_remote_source_subdev(chan);
> -	ret = v4l2_subdev_call(subdev, video, s_stream, false);
> -	if (ret < 0 && ret != -ENOIOCTLCMD)
> -		return ret;
> -
> -	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
> -		return 0;
> -
>  	subdev = tegra_channel_get_remote_csi_subdev(chan);
>  	ret = v4l2_subdev_call(subdev, video, s_stream, false);
>  	if (ret < 0 && ret != -ENOIOCTLCMD)





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux