| Message ID | 20230407133852.2850145-1-luca.ceresoli@bootlin.com |
|---|---|
| Headers | show |
| Series | Add Tegra20 parallel video input capture | expand |
Hello Hans, On Fri, 7 Apr 2023 15:38:32 +0200 Luca Ceresoli <luca.ceresoli@bootlin.com> wrote: > New in v5: dropped the patch that was removing lots of the logic behind > enum_format, after discussion with Hans. The rest is unmodified except for > rebasing and fixing a couple typos in comments. > > Full details follow. > > Tegra20 and other Tegra SoCs have a video input (VI) peripheral that can > receive from either MIPI CSI-2 or parallel video (called respectively "CSI" > and "VIP" in the documentation). The kernel currently has a staging driver > for Tegra210 CSI capture. This patch set adds support for Tegra20 VIP > capture. > > Unfortunately I had no real documentation available to base this work on. > I only had a working downstream 3.1 kernel, so I started with the driver > found there and heavily reworked it to fit into the mainline tegra-video > driver structure. The existing code appears written with the intent of > being modular and allow adding new input mechanisms and new SoCs while > keeping a unique VI core module. However its modularity and extensibility > was not enough to add Tegra20 VIP support, so I added some hooks to turn > hard-coded behaviour into per-SoC or per-bus customizable code. There are > also a fix, some generic cleanups and DT bindings. > > Quick tour of the patches: > > * Device tree bindings > > 01. dt-bindings: display: tegra: add Tegra20 VIP > 02. dt-bindings: display: tegra: vi: add 'vip' property and example > > * Minor improvements to logging, comments, cleanups > > 03. staging: media: tegra-video: improve documentation of tegra_video_format fields > 04. staging: media: tegra-video: document tegra_channel_get_remote_source_subdev > 05. staging: media: tegra-video: fix typos in comment > 06. staging: media: tegra-video: improve error messages > 07. staging: media: tegra-video: slightly simplify cleanup on errors > 08. staging: media: tegra-video: move private struct declaration to C file > 09. staging: media: tegra-video: move tegra210_csi_soc to C file > 10. staging: media: tegra-video: remove unneeded include > > * Preparation to make the VI module generic enough to host Tegra20 and VIP > > 11. staging: media: tegra-video: Kconfig: allow TPG only on Tegra210 > 12. staging: media: tegra-video: move tegra_channel_fmt_align to a per-soc op > 13. staging: media: tegra-video: move default format to soc-specific data > 14. staging: media: tegra-video: move MIPI calibration calls from VI to CSI > 15. staging: media: tegra-video: add a per-soc enable/disable op > 16. staging: media: tegra-video: move syncpt init/free to a per-soc op > 17. staging: media: tegra-video: add syncpts for Tegra20 to struct tegra_vi > 18. staging: media: tegra-video: add hooks for planar YUV and H/V flip > 19. staging: media: tegra-video: add H/V flip controls > > * Implementation of VIP and Tegra20 > > 20. staging: media: tegra-video: add support for Tegra20 parallel input > > Enjoy! > > Changed in v5: > - removed patch 3 as requested by Hans Verkuil; now the driver is kept > video-node-centric and the enum_format logic is unchanged > - rebased on top of that > - trivial fixes (typos) According to your review of v4, removing patch 3 was the only change required, and I didn't do anything else, and there have been no big changes since v1 anyway, so I was wondering whether this series has any hope to make it for 6.4... Best regards, Luca
On 12/04/2023 11:16, Luca Ceresoli wrote: > Hello Hans, > > On Fri, 7 Apr 2023 15:38:32 +0200 > Luca Ceresoli <luca.ceresoli@bootlin.com> wrote: > >> New in v5: dropped the patch that was removing lots of the logic behind >> enum_format, after discussion with Hans. The rest is unmodified except for >> rebasing and fixing a couple typos in comments. >> >> Full details follow. >> >> Tegra20 and other Tegra SoCs have a video input (VI) peripheral that can >> receive from either MIPI CSI-2 or parallel video (called respectively "CSI" >> and "VIP" in the documentation). The kernel currently has a staging driver >> for Tegra210 CSI capture. This patch set adds support for Tegra20 VIP >> capture. >> >> Unfortunately I had no real documentation available to base this work on. >> I only had a working downstream 3.1 kernel, so I started with the driver >> found there and heavily reworked it to fit into the mainline tegra-video >> driver structure. The existing code appears written with the intent of >> being modular and allow adding new input mechanisms and new SoCs while >> keeping a unique VI core module. However its modularity and extensibility >> was not enough to add Tegra20 VIP support, so I added some hooks to turn >> hard-coded behaviour into per-SoC or per-bus customizable code. There are >> also a fix, some generic cleanups and DT bindings. >> >> Quick tour of the patches: >> >> * Device tree bindings >> >> 01. dt-bindings: display: tegra: add Tegra20 VIP >> 02. dt-bindings: display: tegra: vi: add 'vip' property and example >> >> * Minor improvements to logging, comments, cleanups >> >> 03. staging: media: tegra-video: improve documentation of tegra_video_format fields >> 04. staging: media: tegra-video: document tegra_channel_get_remote_source_subdev >> 05. staging: media: tegra-video: fix typos in comment >> 06. staging: media: tegra-video: improve error messages >> 07. staging: media: tegra-video: slightly simplify cleanup on errors >> 08. staging: media: tegra-video: move private struct declaration to C file >> 09. staging: media: tegra-video: move tegra210_csi_soc to C file >> 10. staging: media: tegra-video: remove unneeded include >> >> * Preparation to make the VI module generic enough to host Tegra20 and VIP >> >> 11. staging: media: tegra-video: Kconfig: allow TPG only on Tegra210 >> 12. staging: media: tegra-video: move tegra_channel_fmt_align to a per-soc op >> 13. staging: media: tegra-video: move default format to soc-specific data >> 14. staging: media: tegra-video: move MIPI calibration calls from VI to CSI >> 15. staging: media: tegra-video: add a per-soc enable/disable op >> 16. staging: media: tegra-video: move syncpt init/free to a per-soc op >> 17. staging: media: tegra-video: add syncpts for Tegra20 to struct tegra_vi >> 18. staging: media: tegra-video: add hooks for planar YUV and H/V flip >> 19. staging: media: tegra-video: add H/V flip controls >> >> * Implementation of VIP and Tegra20 >> >> 20. staging: media: tegra-video: add support for Tegra20 parallel input >> >> Enjoy! >> >> Changed in v5: >> - removed patch 3 as requested by Hans Verkuil; now the driver is kept >> video-node-centric and the enum_format logic is unchanged >> - rebased on top of that >> - trivial fixes (typos) > > According to your review of v4, removing patch 3 was the only change > required, and I didn't do anything else, and there have been no big > changes since v1 anyway, so I was wondering whether this series has any > hope to make it for 6.4... It's borderline. I first need to test it again, and I can do that Friday at the earliest (if I find the time). I'll try, but no promises... Regards, Hans
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@bootlin.com> > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > 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)
Hi Hans, On Fri, 14 Apr 2023 17:51:34 +0200 Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@bootlin.com> > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > > > > --- > > > > 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
On 18/04/2023 10:07, Luca Ceresoli wrote: > Hi Hans, > > On Fri, 14 Apr 2023 17:51:34 +0200 > Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@bootlin.com> >>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com> >>> >>> --- >>> >>> 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 >
New in v5: dropped the patch that was removing lots of the logic behind enum_format, after discussion with Hans. The rest is unmodified except for rebasing and fixing a couple typos in comments. Full details follow. Tegra20 and other Tegra SoCs have a video input (VI) peripheral that can receive from either MIPI CSI-2 or parallel video (called respectively "CSI" and "VIP" in the documentation). The kernel currently has a staging driver for Tegra210 CSI capture. This patch set adds support for Tegra20 VIP capture. Unfortunately I had no real documentation available to base this work on. I only had a working downstream 3.1 kernel, so I started with the driver found there and heavily reworked it to fit into the mainline tegra-video driver structure. The existing code appears written with the intent of being modular and allow adding new input mechanisms and new SoCs while keeping a unique VI core module. However its modularity and extensibility was not enough to add Tegra20 VIP support, so I added some hooks to turn hard-coded behaviour into per-SoC or per-bus customizable code. There are also a fix, some generic cleanups and DT bindings. Quick tour of the patches: * Device tree bindings 01. dt-bindings: display: tegra: add Tegra20 VIP 02. dt-bindings: display: tegra: vi: add 'vip' property and example * Minor improvements to logging, comments, cleanups 03. staging: media: tegra-video: improve documentation of tegra_video_format fields 04. staging: media: tegra-video: document tegra_channel_get_remote_source_subdev 05. staging: media: tegra-video: fix typos in comment 06. staging: media: tegra-video: improve error messages 07. staging: media: tegra-video: slightly simplify cleanup on errors 08. staging: media: tegra-video: move private struct declaration to C file 09. staging: media: tegra-video: move tegra210_csi_soc to C file 10. staging: media: tegra-video: remove unneeded include * Preparation to make the VI module generic enough to host Tegra20 and VIP 11. staging: media: tegra-video: Kconfig: allow TPG only on Tegra210 12. staging: media: tegra-video: move tegra_channel_fmt_align to a per-soc op 13. staging: media: tegra-video: move default format to soc-specific data 14. staging: media: tegra-video: move MIPI calibration calls from VI to CSI 15. staging: media: tegra-video: add a per-soc enable/disable op 16. staging: media: tegra-video: move syncpt init/free to a per-soc op 17. staging: media: tegra-video: add syncpts for Tegra20 to struct tegra_vi 18. staging: media: tegra-video: add hooks for planar YUV and H/V flip 19. staging: media: tegra-video: add H/V flip controls * Implementation of VIP and Tegra20 20. staging: media: tegra-video: add support for Tegra20 parallel input Enjoy! Changed in v5: - removed patch 3 as requested by Hans Verkuil; now the driver is kept video-node-centric and the enum_format logic is unchanged - rebased on top of that - trivial fixes (typos) Changed in RESEND,v4: - add Rob's review tag on patch 2 Changed in v4: - fixed the leftovers after the removal of 'channel@0' in DT - added review tags by Dimtry Changed in v3: - removed the 'channel@0' node from the device tree representation of vip - squashed the last two patches (VIP + T20) into one - small cleanups - rebase on v6.2-rc1 Changed in v2: - improved dt-bindings patches based on reviews - removed patches 3 and 4 adding DT labels without a mainline user - two small fixes to the last patch [v4,resend] https://lore.kernel.org/linux-tegra/20230309144320.2937553-1-luca.ceresoli@bootlin.com/ [v4] https://lore.kernel.org/linux-tegra/20230130141603.323221-1-luca.ceresoli@bootlin.com/ [v3] https://lore.kernel.org/linux-media/20221229133205.981397-1-luca.ceresoli@bootlin.com/ [v2] https://lore.kernel.org/linux-tegra/20221222100328.6e341874@booty/T/#t [v1] https://lore.kernel.org/linux-tegra/20221124155634.5bc2a423@booty/T/#t Luca Luca Ceresoli (20): dt-bindings: display: tegra: add Tegra20 VIP dt-bindings: display: tegra: vi: add 'vip' property and example staging: media: tegra-video: improve documentation of tegra_video_format fields staging: media: tegra-video: document tegra_channel_get_remote_source_subdev staging: media: tegra-video: fix typos in comment staging: media: tegra-video: improve error messages staging: media: tegra-video: slightly simplify cleanup on errors staging: media: tegra-video: move private struct declaration to C file staging: media: tegra-video: move tegra210_csi_soc to C file staging: media: tegra-video: remove unneeded include staging: media: tegra-video: Kconfig: allow TPG only on Tegra210 staging: media: tegra-video: move tegra_channel_fmt_align to a per-soc op staging: media: tegra-video: move default format to soc-specific data staging: media: tegra-video: move MIPI calibration calls from VI to CSI staging: media: tegra-video: add a per-soc enable/disable op staging: media: tegra-video: move syncpt init/free to a per-soc op staging: media: tegra-video: add syncpts for Tegra20 to struct tegra_vi staging: media: tegra-video: add hooks for planar YUV and H/V flip staging: media: tegra-video: add H/V flip controls staging: media: tegra-video: add support for Tegra20 parallel input .../display/tegra/nvidia,tegra20-vi.yaml | 59 ++ .../display/tegra/nvidia,tegra20-vip.yaml | 41 ++ MAINTAINERS | 3 + drivers/staging/media/tegra-video/Kconfig | 1 + drivers/staging/media/tegra-video/Makefile | 2 + drivers/staging/media/tegra-video/csi.c | 48 ++ drivers/staging/media/tegra-video/csi.h | 4 - drivers/staging/media/tegra-video/tegra20.c | 661 ++++++++++++++++++ drivers/staging/media/tegra-video/tegra210.c | 90 +++ drivers/staging/media/tegra-video/vi.c | 222 ++---- drivers/staging/media/tegra-video/vi.h | 71 +- drivers/staging/media/tegra-video/video.c | 5 + drivers/staging/media/tegra-video/video.h | 2 +- drivers/staging/media/tegra-video/vip.c | 290 ++++++++ drivers/staging/media/tegra-video/vip.h | 68 ++ 15 files changed, 1380 insertions(+), 187 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-vip.yaml create mode 100644 drivers/staging/media/tegra-video/tegra20.c create mode 100644 drivers/staging/media/tegra-video/vip.c create mode 100644 drivers/staging/media/tegra-video/vip.h