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

[Hans Verkuil]

On 18/04/2023 10:07, Luca Ceresoli wrote:
> Hi Hans,
> 
> On Fri, 14 Apr 2023 17:51:34 +0200
> Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
> 
>> 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.
> 
> Many thanks for your testing! Unfortunately I have no Tegra210 hardware
> so this never happened here, but with your report the problem got
> obvious and, luckily, the fix appeared to be just a oneliner.
> 
> v6 just sent! I'm wondering whether there is still hope to get this
> 6.4...

Sorry, it's too late for 6.4 now. Only fixes can go in for 6.4.

Regards,

	Hans

> 
> Best regards,
> Luca
>