| Message ID | 1596186169-18729-1-git-send-email-skomatineni@nvidia.com |
|---|---|
| Headers | show |
| Series | Support for Tegra video capture from external sensor | expand |
31.07.2020 12:02, Sowjanya Komatineni пишет: ... > @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev) > return ret; > } > > + if (csi_chan->mipi) { > + ret = tegra_mipi_enable(csi_chan->mipi); > + if (ret < 0) { > + dev_err(csi->dev, > + "failed to enable MIPI pads: %d\n", ret); > + goto rpm_put; > + } > + > + /* > + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to > + * be calibrated after power on. > + * So, trigger the calibration start here and results will > + * be latched and applied to the pads when link is in LP11 > + * state during start of sensor streaming. > + */ > + ret = tegra_mipi_start_calibration(csi_chan->mipi); > + if (ret < 0) { > + dev_err(csi->dev, > + "failed to start MIPI calibration: %d\n", ret); > + goto disable_mipi; > + } What would happen if CSI stream is enabled and then immediately disabled without enabling camera sensor? > + } > + ... > 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; > > /* > @@ -206,13 +207,30 @@ static int tegra_channel_enable_stream(struct tegra_vi_channel *chan) > 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 > + * and in case of sensor stream-on failure, cancel the calibration. > + */ > src_subdev = tegra_channel_get_remote_source_subdev(chan); Is it possible to move the start_calibration() here? > ret = v4l2_subdev_call(src_subdev, video, s_stream, true); > if (ret < 0 && ret != -ENOIOCTLCMD) { > + tegra_mipi_cancel_calibration(csi_chan->mipi); > v4l2_subdev_call(csi_subdev, video, s_stream, false); > return ret; > } > > + ret = tegra_mipi_finish_calibration(csi_chan->mipi); > + if (ret < 0) > + dev_warn(csi_chan->csi->dev, > + "MIPI calibration failed: %d\n", ret); > + > return 0; > } > >
31.07.2020 12:02, Sowjanya Komatineni пишет: > This patch separates implementation of CSI stream enable and disable > into separate functions for readability. > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > drivers/staging/media/tegra-video/csi.c | 51 ++++++++++++++++++++++----------- > 1 file changed, 35 insertions(+), 16 deletions(-) > > diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c > index fb667df..cfe6187 100644 > --- a/drivers/staging/media/tegra-video/csi.c > +++ b/drivers/staging/media/tegra-video/csi.c > @@ -232,34 +232,53 @@ static int tegra_csi_g_frame_interval(struct v4l2_subdev *subdev, > return 0; > } > > -static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable) > +static int tegra_csi_enable_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 ret = 0; > + int ret; > + > + ret = pm_runtime_get_sync(csi->dev); > + if (ret < 0) { > + dev_err(csi->dev, "failed to get runtime PM: %d\n", ret); > + pm_runtime_put_noidle(csi->dev); > + return ret; > + } > > csi_chan->pg_mode = chan->pg_mode; > - if (enable) { > - ret = pm_runtime_get_sync(csi->dev); > - if (ret < 0) { > - dev_err(csi->dev, > - "failed to get runtime PM: %d\n", ret); > - pm_runtime_put_noidle(csi->dev); > - return ret; > - } > + ret = csi->ops->csi_start_streaming(csi_chan); > + if (ret < 0) > + goto rpm_put; > > - ret = csi->ops->csi_start_streaming(csi_chan); > - if (ret < 0) > - goto rpm_put; > + return 0; > > - return 0; > - } > +rpm_put: > + pm_runtime_put(csi->dev); > + return ret; > +} > + > +static int tegra_csi_disable_stream(struct v4l2_subdev *subdev) > +{ > + struct tegra_csi_channel *csi_chan = to_csi_chan(subdev); > + struct tegra_csi *csi = csi_chan->csi; > > csi->ops->csi_stop_streaming(csi_chan); > > -rpm_put: > pm_runtime_put(csi->dev); > + > + return 0; > +} > + > +static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable) > +{ > + int ret; > + > + if (enable) > + ret = tegra_csi_enable_stream(subdev); > + else > + ret = tegra_csi_disable_stream(subdev); > + > return ret; > } > > Thanks! Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
On Fri, Jul 31, 2020 at 02:02:39AM -0700, Sowjanya Komatineni wrote: > This series adds support for video capture from external camera sensor to > Tegra video driver. No need anymore to CC me or the i2c-list, I think? Also, is this series really still RFC?
On 7/31/20 4:39 AM, Dmitry Osipenko wrote: > 31.07.2020 12:02, Sowjanya Komatineni пишет: > ... >> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev) >> return ret; >> } >> >> + if (csi_chan->mipi) { >> + ret = tegra_mipi_enable(csi_chan->mipi); >> + if (ret < 0) { >> + dev_err(csi->dev, >> + "failed to enable MIPI pads: %d\n", ret); >> + goto rpm_put; >> + } >> + >> + /* >> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to >> + * be calibrated after power on. >> + * So, trigger the calibration start here and results will >> + * be latched and applied to the pads when link is in LP11 >> + * state during start of sensor streaming. >> + */ >> + ret = tegra_mipi_start_calibration(csi_chan->mipi); >> + if (ret < 0) { >> + dev_err(csi->dev, >> + "failed to start MIPI calibration: %d\n", ret); >> + goto disable_mipi; >> + } > What would happen if CSI stream is enabled and then immediately disabled > without enabling camera sensor? Nothing will happen as during stream enable csi receiver is kept ready. But actual capture will not happen during that point. > >> + } >> + > ... >> 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; >> >> /* >> @@ -206,13 +207,30 @@ static int tegra_channel_enable_stream(struct tegra_vi_channel *chan) >> 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 >> + * and in case of sensor stream-on failure, cancel the calibration. >> + */ >> src_subdev = tegra_channel_get_remote_source_subdev(chan); > Is it possible to move the start_calibration() here? I think we can do start here as well I guess but currently I am following steps order as per design document. This is the reason I have updated in above comment as well. > >> ret = v4l2_subdev_call(src_subdev, video, s_stream, true); >> if (ret < 0 && ret != -ENOIOCTLCMD) { >> + tegra_mipi_cancel_calibration(csi_chan->mipi); >> v4l2_subdev_call(csi_subdev, video, s_stream, false); >> return ret; >> } >> >> + ret = tegra_mipi_finish_calibration(csi_chan->mipi); >> + if (ret < 0) >> + dev_warn(csi_chan->csi->dev, >> + "MIPI calibration failed: %d\n", ret); >> + >> return 0; >> } >> >>
Sorry Wolfram. Will remove from CC list on my next replies.. On 7/31/20 6:40 AM, Wolfram Sang wrote: > On Fri, Jul 31, 2020 at 02:02:39AM -0700, Sowjanya Komatineni wrote: >> This series adds support for video capture from external camera sensor to >> Tegra video driver. > No need anymore to CC me or the i2c-list, I think? Also, is this series > really still RFC?
31.07.2020 18:46, Sowjanya Komatineni пишет: > > On 7/31/20 4:39 AM, Dmitry Osipenko wrote: >> 31.07.2020 12:02, Sowjanya Komatineni пишет: >> ... >>> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct >>> v4l2_subdev *subdev) >>> return ret; >>> } >>> + if (csi_chan->mipi) { >>> + ret = tegra_mipi_enable(csi_chan->mipi); >>> + if (ret < 0) { >>> + dev_err(csi->dev, >>> + "failed to enable MIPI pads: %d\n", ret); >>> + goto rpm_put; >>> + } >>> + >>> + /* >>> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to >>> + * be calibrated after power on. >>> + * So, trigger the calibration start here and results will >>> + * be latched and applied to the pads when link is in LP11 >>> + * state during start of sensor streaming. >>> + */ >>> + ret = tegra_mipi_start_calibration(csi_chan->mipi); >>> + if (ret < 0) { >>> + dev_err(csi->dev, >>> + "failed to start MIPI calibration: %d\n", ret); >>> + goto disable_mipi; >>> + } >> What would happen if CSI stream is enabled and then immediately disabled >> without enabling camera sensor? > > Nothing will happen as during stream enable csi receiver is kept ready. > > But actual capture will not happen during that point. Could you please show how the full call chain looks like? It's not clear to me what keeps CSI stream "ready".
On 7/31/20 9:14 AM, Dmitry Osipenko wrote: > 31.07.2020 18:46, Sowjanya Komatineni пишет: >> On 7/31/20 4:39 AM, Dmitry Osipenko wrote: >>> 31.07.2020 12:02, Sowjanya Komatineni пишет: >>> ... >>>> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct >>>> v4l2_subdev *subdev) >>>> return ret; >>>> } >>>> + if (csi_chan->mipi) { >>>> + ret = tegra_mipi_enable(csi_chan->mipi); >>>> + if (ret < 0) { >>>> + dev_err(csi->dev, >>>> + "failed to enable MIPI pads: %d\n", ret); >>>> + goto rpm_put; >>>> + } >>>> + >>>> + /* >>>> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to >>>> + * be calibrated after power on. >>>> + * So, trigger the calibration start here and results will >>>> + * be latched and applied to the pads when link is in LP11 >>>> + * state during start of sensor streaming. >>>> + */ >>>> + ret = tegra_mipi_start_calibration(csi_chan->mipi); >>>> + if (ret < 0) { >>>> + dev_err(csi->dev, >>>> + "failed to start MIPI calibration: %d\n", ret); >>>> + goto disable_mipi; >>>> + } >>> What would happen if CSI stream is enabled and then immediately disabled >>> without enabling camera sensor? >> Nothing will happen as during stream enable csi receiver is kept ready. >> >> But actual capture will not happen during that point. > Could you please show how the full call chain looks like? It's not clear > to me what keeps CSI stream "ready". VI is the main video input (video device) and on streaming it starts stream of CSI subdev prior to stream of Sensor. HW path, sensor stream (CSI TX) -> CSI stream (RX) During CSI stream on, CSI PHY receiver is enabled to start receiving the data but internally capture assembled to active state will happen only when Tegra VI single shot is issues where VI thru pixel parser gets captures data into the memory
31.07.2020 19:29, Sowjanya Komatineni пишет: > > On 7/31/20 9:14 AM, Dmitry Osipenko wrote: >> 31.07.2020 18:46, Sowjanya Komatineni пишет: >>> On 7/31/20 4:39 AM, Dmitry Osipenko wrote: >>>> 31.07.2020 12:02, Sowjanya Komatineni пишет: >>>> ... >>>>> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct >>>>> v4l2_subdev *subdev) >>>>> return ret; >>>>> } >>>>> + if (csi_chan->mipi) { >>>>> + ret = tegra_mipi_enable(csi_chan->mipi); >>>>> + if (ret < 0) { >>>>> + dev_err(csi->dev, >>>>> + "failed to enable MIPI pads: %d\n", ret); >>>>> + goto rpm_put; >>>>> + } >>>>> + >>>>> + /* >>>>> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to >>>>> + * be calibrated after power on. >>>>> + * So, trigger the calibration start here and results will >>>>> + * be latched and applied to the pads when link is in LP11 >>>>> + * state during start of sensor streaming. >>>>> + */ >>>>> + ret = tegra_mipi_start_calibration(csi_chan->mipi); >>>>> + if (ret < 0) { >>>>> + dev_err(csi->dev, >>>>> + "failed to start MIPI calibration: %d\n", ret); >>>>> + goto disable_mipi; >>>>> + } >>>> What would happen if CSI stream is enabled and then immediately >>>> disabled >>>> without enabling camera sensor? >>> Nothing will happen as during stream enable csi receiver is kept ready. >>> >>> But actual capture will not happen during that point. >> Could you please show how the full call chain looks like? It's not clear >> to me what keeps CSI stream "ready". > > VI is the main video input (video device) and on streaming it starts > stream of CSI subdev prior to stream of Sensor. > > HW path, sensor stream (CSI TX) -> CSI stream (RX) > > During CSI stream on, CSI PHY receiver is enabled to start receiving the > data but internally capture assembled to active state will happen only > when Tegra VI single shot is issues where VI thru pixel parser gets > captures data into the memory Alright, I see now. Will be great if you could change this hunk: { ret = v4l2_subdev_call(src_subdev, video, s_stream, true); if (ret < 0 && ret != -ENOIOCTLCMD) { tegra_mipi_cancel_calibration(csi_chan->mipi); v4l2_subdev_call(csi_subdev, video, s_stream, false); return ret; } } to look like this: { err = v4l2_subdev_call(src_subdev, video, s_stream, true); if (err < 0 && err != -ENOIOCTLCMD) goto err_disable_csi_stream; ... return 0; err_disable_csi_stream: tegra_mipi_cancel_calibration(csi_chan->mipi); v4l2_subdev_call(csi_subdev, video, s_stream, false); return err; } It should make code a bit easier to read and follow. Otherwise this patch looks good to me, thanks.
On 7/31/20 1:42 PM, Dmitry Osipenko wrote: > 31.07.2020 19:29, Sowjanya Komatineni пишет: >> On 7/31/20 9:14 AM, Dmitry Osipenko wrote: >>> 31.07.2020 18:46, Sowjanya Komatineni пишет: >>>> On 7/31/20 4:39 AM, Dmitry Osipenko wrote: >>>>> 31.07.2020 12:02, Sowjanya Komatineni пишет: >>>>> ... >>>>>> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct >>>>>> v4l2_subdev *subdev) >>>>>> return ret; >>>>>> } >>>>>> + if (csi_chan->mipi) { >>>>>> + ret = tegra_mipi_enable(csi_chan->mipi); >>>>>> + if (ret < 0) { >>>>>> + dev_err(csi->dev, >>>>>> + "failed to enable MIPI pads: %d\n", ret); >>>>>> + goto rpm_put; >>>>>> + } >>>>>> + >>>>>> + /* >>>>>> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to >>>>>> + * be calibrated after power on. >>>>>> + * So, trigger the calibration start here and results will >>>>>> + * be latched and applied to the pads when link is in LP11 >>>>>> + * state during start of sensor streaming. >>>>>> + */ >>>>>> + ret = tegra_mipi_start_calibration(csi_chan->mipi); >>>>>> + if (ret < 0) { >>>>>> + dev_err(csi->dev, >>>>>> + "failed to start MIPI calibration: %d\n", ret); >>>>>> + goto disable_mipi; >>>>>> + } >>>>> What would happen if CSI stream is enabled and then immediately >>>>> disabled >>>>> without enabling camera sensor? >>>> Nothing will happen as during stream enable csi receiver is kept ready. >>>> >>>> But actual capture will not happen during that point. >>> Could you please show how the full call chain looks like? It's not clear >>> to me what keeps CSI stream "ready". >> VI is the main video input (video device) and on streaming it starts >> stream of CSI subdev prior to stream of Sensor. >> >> HW path, sensor stream (CSI TX) -> CSI stream (RX) >> >> During CSI stream on, CSI PHY receiver is enabled to start receiving the >> data but internally capture assembled to active state will happen only >> when Tegra VI single shot is issues where VI thru pixel parser gets >> captures data into the memory > Alright, I see now. > > Will be great if you could change this hunk: > > { > ret = v4l2_subdev_call(src_subdev, video, s_stream, true); > if (ret < 0 && ret != -ENOIOCTLCMD) { > tegra_mipi_cancel_calibration(csi_chan->mipi); > v4l2_subdev_call(csi_subdev, video, s_stream, false); > return ret; > } > } > > to look like this: > > { > err = v4l2_subdev_call(src_subdev, video, s_stream, true); > if (err < 0 && err != -ENOIOCTLCMD) > goto err_disable_csi_stream; > ... > return 0; > > err_disable_csi_stream: > tegra_mipi_cancel_calibration(csi_chan->mipi); > > v4l2_subdev_call(csi_subdev, video, s_stream, false); > > return err; > } > > > It should make code a bit easier to read and follow. > > Otherwise this patch looks good to me, thanks. Thanks Dmitry. Will send v7 now with this minor fix and would like to close on this soon. Sowjanya
This series adds support for video capture from external camera sensor to Tegra video driver. Jetson TX1 has camera expansion connector and supports custom camera module designed as per TX1 design specification. This series also enables camera capture support for Jetson Nano which has Raspberry PI camera header. This series is tested with IMX274 and IMX219 camera sensors. This series include, Tegra video driver updates - TPG Vs Non-TPG based on Kconfig - Support for external sensor video capture based on device graph from DT - Support for selection ioctl operations - Tegra MIPI CSI pads calibration - CSI T-CLK and T-HS settle time computation based on clock rates - dt-binding doc update Host1x driver updates - Keep MIPI clock enabled till calibration is done Delta between patch versions: [v6]: Includes below changes based on v5 feedback - Patches are based on latest linux-next. - separated stream enable and disable implementations into separate functions for tegra_channel_set_stream() and tegra_csi_s_stream(). - changed dev_err to dev_warn on MIPI calibration failure after sensor streaming as its not critical error. Note: Patch-0006 has compilation dependency on https://patchwork.kernel.org/patch/11659521/ [v5]: Includes below minor change based on v4 feedback Patch-0012: renames APIs to use calibration instead of calibrate. Note: Patch-0010 has compilation dependency on https://patchwork.kernel.org/patch/11659521/ [v4]: Includes below fix based on v3 feedback - Patches are based on latest linux-next. - With split of tegra_mipi_calibrate() and tegra_mipi_wait(), mipi clock is not left enabled till calibration done. This series adds a patch to fix this by keeping clock enabled till calibration is done. Note: Patch-0010 has compilation dependency on https://patchwork.kernel.org/patch/11659521/ [v3]: Includes v2 feedback - Uses separate helper function for retrieving remote csi subdevice and source subdevice. - Added check for presence of subdevice ops set/get_selection - dropped vb2_queue_release from driver and using vb2_video_unregister_device instead of video_unregister_device. - video device register should happen in the last after all video device related setup is done in the driver. This is being addressed in below RFC patch. Once proper implementation of this is available will update Tegra video driver to use split APIs and do all setup prior to device register. Added this as TODO in the driver. https://www.spinics.net/lists/linux-media/msg172761.html Note: Patch-0012 has compilation dependency on https://patchwork.kernel.org/patch/11659521/ [v2]: Includes below changes based on v1 feedback - dt-binding document and the driver update for device graph to use separate ports for sink endpoint and source endpoint for csi. - Use data-lanes endpoint property for csi. - Update tegra_mipi_request() to take device node pointer argument rather than adding extra API. - Remove checking for clk pointer before clk_disable. Sowjanya Komatineni (10): media: tegra-video: Fix channel format alignment media: tegra-video: Enable TPG based on kernel config media: tegra-video: Update format lookup to offset based dt-bindings: tegra: Update VI and CSI bindings with port info media: tegra-video: Separate CSI stream enable and disable implementations media: tegra-video: Add support for external sensor capture media: tegra-video: Add support for selection ioctl ops gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done media: tegra-video: Add CSI MIPI pads calibration media: tegra-video: Compute settle times based on the clock rate .../display/tegra/nvidia,tegra20-host1x.txt | 92 ++- drivers/gpu/drm/tegra/dsi.c | 4 +- drivers/gpu/host1x/mipi.c | 19 +- drivers/staging/media/tegra-video/Kconfig | 7 + drivers/staging/media/tegra-video/TODO | 6 - drivers/staging/media/tegra-video/csi.c | 305 +++++++- drivers/staging/media/tegra-video/csi.h | 8 + drivers/staging/media/tegra-video/tegra210.c | 25 +- drivers/staging/media/tegra-video/vi.c | 834 +++++++++++++++++++-- drivers/staging/media/tegra-video/vi.h | 25 +- drivers/staging/media/tegra-video/video.c | 23 +- include/linux/host1x.h | 5 +- 12 files changed, 1237 insertions(+), 116 deletions(-)