lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Feb 2020 21:50:18 -0800 From: Sowjanya Komatineni <skomatineni@...dia.com> To: Hans Verkuil <hverkuil@...all.nl>, <thierry.reding@...il.com>, <jonathanh@...dia.com>, <frankc@...dia.com>, <helen.koike@...labora.com>, <sboyd@...nel.org> CC: <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>, <linux-clk@...r.kernel.org>, <linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org> Subject: Re: [RFC PATCH v3 4/6] media: tegra: Add Tegra210 Video input driver On 2/25/20 8:49 PM, Sowjanya Komatineni wrote: > > On 2/20/20 4:44 AM, Hans Verkuil wrote: >> External email: Use caution opening links or attachments >> >> >> Hi Sowjanya, >> >> Some code review comments below: >> >> On 2/14/20 7:23 PM, Sowjanya Komatineni wrote: >>> Tegra210 contains a powerful Video Input (VI) hardware controller >>> which can support up to 6 MIPI CSI camera sensors. >>> >>> Each Tegra CSI port can be one-to-one mapped to VI channel and can >>> capture from an external camera sensor connected to CSI or from >>> built-in test pattern generator. >>> >>> Tegra210 supports built-in test pattern generator from CSI to VI. >>> >>> This patch adds a V4L2 media controller and capture driver support >>> for Tegra210 built-in CSI to VI test pattern generator. >>> >>> Signed-off-by: Sowjanya Komatineni <skomatineni@...dia.com> >>> --- >>> drivers/staging/media/Kconfig | 2 + >>> drivers/staging/media/Makefile | 1 + >>> drivers/staging/media/tegra/Kconfig | 10 + >>> drivers/staging/media/tegra/Makefile | 8 + >>> drivers/staging/media/tegra/TODO | 10 + >>> drivers/staging/media/tegra/tegra-common.h | 239 +++++++ >>> drivers/staging/media/tegra/tegra-csi.c | 374 ++++++++++ >>> drivers/staging/media/tegra/tegra-csi.h | 115 ++++ >>> drivers/staging/media/tegra/tegra-vi.c | 1019 >>> ++++++++++++++++++++++++++++ >>> drivers/staging/media/tegra/tegra-vi.h | 79 +++ >>> drivers/staging/media/tegra/tegra-video.c | 118 ++++ >>> drivers/staging/media/tegra/tegra-video.h | 32 + >>> drivers/staging/media/tegra/tegra210.c | 767 >>> +++++++++++++++++++++ >>> drivers/staging/media/tegra/tegra210.h | 190 ++++++ >>> 14 files changed, 2964 insertions(+) >>> create mode 100644 drivers/staging/media/tegra/Kconfig >>> create mode 100644 drivers/staging/media/tegra/Makefile >>> create mode 100644 drivers/staging/media/tegra/TODO >>> create mode 100644 drivers/staging/media/tegra/tegra-common.h >>> create mode 100644 drivers/staging/media/tegra/tegra-csi.c >>> create mode 100644 drivers/staging/media/tegra/tegra-csi.h >>> create mode 100644 drivers/staging/media/tegra/tegra-vi.c >>> create mode 100644 drivers/staging/media/tegra/tegra-vi.h >>> create mode 100644 drivers/staging/media/tegra/tegra-video.c >>> create mode 100644 drivers/staging/media/tegra/tegra-video.h >>> create mode 100644 drivers/staging/media/tegra/tegra210.c >>> create mode 100644 drivers/staging/media/tegra/tegra210.h >>> > >>> +static int chan_capture_kthread_done(void *data) >>> +{ >>> + struct tegra_vi_channel *chan = data; >>> + struct tegra_channel_buffer *buf; >>> + >>> + set_freezable(); >>> + >>> + while (1) { >>> + try_to_freeze(); >>> + >>> + wait_event_interruptible(chan->done_wait, >>> + !list_empty(&chan->done) || >>> + kthread_should_stop()); >>> + >>> + if (kthread_should_stop()) >>> + break; >> I think it makes more sense if this test is moved to the end... >> >>> + >>> + buf = dequeue_buf_done(chan); >>> + if (!buf) >>> + continue; >> ... and this becomes: >> >> if (buf) >>> + tegra_channel_capture_done(chan, buf); >> This change simplifies stop_streaming (see below). > > With kthread_should_stop check at end, I see sometimes outstanding > buffer in done queue by the time threads are stopped during stream stop. > > When I run compliance stream io tests continuously in loop, depending > on time of stream stop request capture thread terminated after > initiating frame capture and moving buffer to done queue while done > thread was still in wait for previous MW_ACK and on seeing > kthread_should_stop done thread terminated with last buffer left in > done queue. > > So looks like we need to keep checking for outstanding buffer and > handle it during stop streaming like in v3. > Will change in v4 to handle all pending done queue buffers before terminating thread. > >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +int tegra210_vi_start_streaming(struct vb2_queue *vq, u32 count) >>> +{ >>> + struct tegra_vi_channel *chan = vb2_get_drv_priv(vq); >>> + struct media_pipeline *pipe = &chan->video.pipe; >>> + int ret = 0; >>> + >>> + tegra_vi_write(chan, TEGRA_VI_CFG_CG_CTRL, VI_CG_2ND_LEVEL_EN); >>> + >>> + /* clear errors */ >>> + vi_csi_write(chan, TEGRA_VI_CSI_ERROR_STATUS, 0xFFFFFFFF); >>> + >>> + /* >>> + * Sync point FIFO full stalls the host interface. >>> + * Setting NO_STALL will drop INCR_SYNCPT methods when fifos are >>> + * full and the corresponding condition bits in INCR_SYNCPT_ERROR >>> + * register will be set. >>> + * This allows SW to process error recovery. >>> + */ >>> + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT_CNTRL, >>> + TEGRA_VI_CFG_VI_INCR_SYNCPT_NO_STALL); >>> + >>> + /* start the pipeline */ >>> + ret = media_pipeline_start(&chan->video.entity, pipe); >>> + if (ret < 0) >>> + goto error_pipeline_start; >>> + >>> + /* program VI registers after TPG, sensors and CSI streaming */ >>> + ret = tegra_channel_set_stream(chan, true); >>> + if (ret < 0) >>> + goto error_set_stream; >>> + >>> + tegra_channel_capture_setup(chan); >>> + >>> + chan->sequence = 0; >>> + >>> + /* start kthreads to capture data to buffer and return them */ >>> + chan->kthread_capture_done = >>> kthread_run(chan_capture_kthread_done, >>> + chan, "%s:1", >>> + chan->video.name); >>> + if (IS_ERR(chan->kthread_capture_done)) { >>> + ret = PTR_ERR(chan->kthread_capture_done); >>> + chan->kthread_capture_done = NULL; >>> + dev_err(&chan->video.dev, >>> + "failed capture done kthread: %d\n", ret); >>> + goto error_kthread_done; >>> + } >>> + >>> + chan->kthread_capture_start = >>> kthread_run(chan_capture_kthread_start, >>> + chan, "%s:0", >>> + chan->video.name); >>> + if (IS_ERR(chan->kthread_capture_start)) { >>> + ret = PTR_ERR(chan->kthread_capture_start); >>> + chan->kthread_capture_start = NULL; >>> + dev_err(&chan->video.dev, >>> + "failed capture start kthread: %d\n", ret); >>> + goto error_kthread_start; >>> + } >>> + >>> + return 0; >>> + >>> +error_kthread_start: >>> + kthread_stop(chan->kthread_capture_done); >>> +error_kthread_done: >>> + tegra_channel_set_stream(chan, false); >>> +error_set_stream: >>> + media_pipeline_stop(&chan->video.entity); >>> +error_pipeline_start: >>> + vq->start_streaming_called = 0; >>> + tegra_channel_release_queued_buffers(chan, VB2_BUF_STATE_QUEUED); >>> + return ret; >>> +} >>> + >>> +void tegra210_vi_stop_streaming(struct vb2_queue *vq) >>> +{ >>> + struct tegra_vi_channel *chan = vb2_get_drv_priv(vq); >>> + struct tegra_channel_buffer *buf; >>> + >>> + if (!chan->kthread_capture_start || !chan->kthread_capture_done) >>> + return; >>> + >>> + kthread_stop(chan->kthread_capture_start); >>> + chan->kthread_capture_start = NULL; >>> + kthread_stop(chan->kthread_capture_done); >>> + chan->kthread_capture_done = NULL; >>> + >> With the change in chan_capture_kthread_done() as described above you >> can >> drop the next 4 lines since that's guaranteed to be done by the thread. >> >>> + /* wait for last frame MW_ACK_DONE */ >>> + buf = dequeue_buf_done(chan); >>> + if (buf) >>> + tegra_channel_capture_done(chan, buf); >>> + >>> + tegra_channel_release_queued_buffers(chan, VB2_BUF_STATE_ERROR); >>> + >>> + tegra_channel_set_stream(chan, false); >>> + >>> + /* disable clock gating to enable continuous clock */ >>> + tegra_vi_write(chan, TEGRA_VI_CFG_CG_CTRL, 0); >>> + >>> + /* reset VI MCIF, PF, SENSORCTL, and SHADOW logic */ >>> + vi_csi_write(chan, TEGRA_VI_CSI_SW_RESET, 0xF); >>> + vi_csi_write(chan, TEGRA_VI_CSI_SW_RESET, 0x0); >>> + vi_csi_write(chan, TEGRA_VI_CSI_IMAGE_DEF, 0); >>> + >>> + /* enable clock gating so VI can be clock gated if necessary */ >>> + tegra_vi_write(chan, TEGRA_VI_CFG_CG_CTRL, VI_CG_2ND_LEVEL_EN); >>> + vi_csi_write(chan, TEGRA_VI_CSI_ERROR_STATUS, 0xFFFFFFFF); >>> + >>> + media_pipeline_stop(&chan->video.entity); >>> +} >>> + >>> +void tegra210_csi_error_recover(struct tegra_csi_channel *csi_chan) >> >> Regards, >> >> Hans
Powered by blists - more mailing lists