Hi Hans, On Fri, 14 Apr 2023 17:51:34 +0200 Hans Verkuil <[email protected]> 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 <[email protected]> > > Reviewed-by: Dmitry Osipenko <[email protected]> > > > > --- > > > > 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... Best regards, Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
