| Message ID | 1585963507-12610-1-git-send-email-skomatineni@nvidia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Tegra driver for video capture | expand |
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +static int tegra_csi_init(struct host1x_client *client) > +{ > + struct tegra_csi *csi = host1x_client_to_csi(client); > + struct tegra_video_device *vid = dev_get_drvdata(client->host); > + int ret; > + > + vid->csi = csi; > + > + INIT_LIST_HEAD(&csi->csi_chans); > + > + if (pm_runtime_enabled(csi->dev)) { > + 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; > + } > + } else { RPM is supposed to be always available on Tegra nowadays.
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +static int tegra_vi_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct tegra_vi *vi; > + int ret; > + > + vi = kzalloc(sizeof(*vi), GFP_KERNEL); devm_kzalloc()? > + if (!vi) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + vi->iomem = devm_ioremap_resource(&pdev->dev, res); devm_platform_ioremap_resource()? > + if (IS_ERR(vi->iomem)) { > + ret = PTR_ERR(vi->iomem); > + goto cleanup; > + } > + > + vi->soc = of_device_get_match_data(&pdev->dev); This can't fail because match already happened. > + if (!vi->soc) { > + ret = -ENODATA; > + goto cleanup; > + } > + > + vi->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(vi->clk)) { > + ret = PTR_ERR(vi->clk); > + dev_err(&pdev->dev, "failed to get vi clock: %d\n", ret); > + goto cleanup; > + } > + > + vi->vdd = devm_regulator_get(&pdev->dev, "avdd-dsi-csi"); > + if (IS_ERR(vi->vdd)) { > + ret = PTR_ERR(vi->vdd); > + dev_err(&pdev->dev, "failed to get VDD supply: %d\n", ret); > + goto cleanup; > + } > + > + if (!pdev->dev.pm_domain) { > + ret = -ENOENT; > + dev_warn(&pdev->dev, "PM domain is not attached: %d\n", ret); > + goto cleanup; > + } > + > + ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > + if (ret) { > + dev_err(&pdev->dev, > + "failed to populate vi child device: %d\n", ret); > + goto cleanup; > + } > + > + vi->dev = &pdev->dev; > + vi->ops = vi->soc->ops; > + platform_set_drvdata(pdev, vi); > + pm_runtime_enable(&pdev->dev); > + > + /* initialize host1x interface */ > + INIT_LIST_HEAD(&vi->client.list); > + vi->client.ops = &vi_client_ops; > + vi->client.dev = &pdev->dev; > + > + ret = host1x_client_register(&vi->client); > + if (ret < 0) { > + dev_err(vi->dev, > + "failed to register host1x client: %d\n", ret); > + ret = -ENODEV; > + goto rpm_disable; > + } > + > + return 0; > + > +rpm_disable: > + pm_runtime_disable(&pdev->dev); > + of_platform_depopulate(vi->dev); > +cleanup: > + kfree(vi); > + return ret; > +} > + > +static int tegra_vi_remove(struct platform_device *pdev) > +{ > + struct tegra_vi *vi = platform_get_drvdata(pdev); > + int err; > + > + pm_runtime_disable(vi->dev); > + > + err = host1x_client_unregister(&vi->client); > + if (err < 0) { > + dev_err(vi->dev, > + "failed to unregister host1x client: %d\n", err); > + return err; > + } The removal order should be opposite to the registration order.
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +/* Tegra210 VI registers accessors */ > +static void tegra_vi_write(struct tegra_vi_channel *chan, unsigned int addr, > + u32 val) > +{ > + writel(val, chan->vi->iomem + addr); > +} > + > +static u32 tegra_vi_read(struct tegra_vi_channel *chan, unsigned int addr) > +{ > + return readl(chan->vi->iomem + addr); > +} ... Perhaps all reads and writes should be relaxed?
05.04.2020 22:45, Dmitry Osipenko пишет: > 04.04.2020 04:25, Sowjanya Komatineni пишет: > ... >> +static int tegra_vi_probe(struct platform_device *pdev) >> +{ >> + struct resource *res; >> + struct tegra_vi *vi; >> + int ret; >> + >> + vi = kzalloc(sizeof(*vi), GFP_KERNEL); > > devm_kzalloc()? > >> + if (!vi) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + vi->iomem = devm_ioremap_resource(&pdev->dev, res); > > devm_platform_ioremap_resource()? > >> + if (IS_ERR(vi->iomem)) { >> + ret = PTR_ERR(vi->iomem); >> + goto cleanup; >> + } >> + >> + vi->soc = of_device_get_match_data(&pdev->dev); > > This can't fail because match already happened. > >> + if (!vi->soc) { >> + ret = -ENODATA; >> + goto cleanup; >> + } >> + >> + vi->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(vi->clk)) { >> + ret = PTR_ERR(vi->clk); >> + dev_err(&pdev->dev, "failed to get vi clock: %d\n", ret); >> + goto cleanup; >> + } >> + >> + vi->vdd = devm_regulator_get(&pdev->dev, "avdd-dsi-csi"); >> + if (IS_ERR(vi->vdd)) { >> + ret = PTR_ERR(vi->vdd); >> + dev_err(&pdev->dev, "failed to get VDD supply: %d\n", ret); >> + goto cleanup; >> + } >> + >> + if (!pdev->dev.pm_domain) { >> + ret = -ENOENT; >> + dev_warn(&pdev->dev, "PM domain is not attached: %d\n", ret); >> + goto cleanup; >> + } >> + >> + ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); >> + if (ret) { >> + dev_err(&pdev->dev, >> + "failed to populate vi child device: %d\n", ret); >> + goto cleanup; >> + } >> + >> + vi->dev = &pdev->dev; >> + vi->ops = vi->soc->ops; >> + platform_set_drvdata(pdev, vi); >> + pm_runtime_enable(&pdev->dev); >> + >> + /* initialize host1x interface */ >> + INIT_LIST_HEAD(&vi->client.list); >> + vi->client.ops = &vi_client_ops; >> + vi->client.dev = &pdev->dev; >> + >> + ret = host1x_client_register(&vi->client); >> + if (ret < 0) { >> + dev_err(vi->dev, >> + "failed to register host1x client: %d\n", ret); >> + ret = -ENODEV; >> + goto rpm_disable; >> + } >> + >> + return 0; >> + >> +rpm_disable: >> + pm_runtime_disable(&pdev->dev); >> + of_platform_depopulate(vi->dev); >> +cleanup: >> + kfree(vi); >> + return ret; >> +} >> + >> +static int tegra_vi_remove(struct platform_device *pdev) >> +{ >> + struct tegra_vi *vi = platform_get_drvdata(pdev); >> + int err; >> + >> + pm_runtime_disable(vi->dev); >> + >> + err = host1x_client_unregister(&vi->client); >> + if (err < 0) { >> + dev_err(vi->dev, >> + "failed to unregister host1x client: %d\n", err); >> + return err; >> + } > > The removal order should be opposite to the registration order. > All the same to the tegra_csi, btw.
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan, > + struct tegra_channel_buffer *buf) > +{ > + int err = 0; > + u32 thresh, value, frame_start, mw_ack_done; > + int bytes_per_line = chan->format.bytesperline; > + > + /* program buffer address by using surface 0 */ > + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, > + (u64)buf->addr >> 32); > + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr); > + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line); > + > + /* > + * Tegra VI block interacts with host1x syncpt for synchronizing > + * programmed condition of capture state and hardware operation. > + * Frame start and Memory write acknowledge syncpts has their own > + * FIFO of depth 2. > + * > + * Syncpoint trigger conditions set through VI_INCR_SYNCPT register > + * are added to HW syncpt FIFO and when the HW triggers, syncpt > + * condition is removed from the FIFO and counter at syncpoint index > + * will be incremented by the hardware and software can wait for > + * counter to reach threshold to synchronize capturing frame with the > + * hardware capture events. > + */ > + > + /* increase channel syncpoint threshold for FRAME_START */ > + thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1); > + > + /* Program FRAME_START trigger condition syncpt request */ > + frame_start = VI_CSI_PP_FRAME_START(chan->portno); > + value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) | > + host1x_syncpt_id(chan->frame_start_sp); > + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); > + > + /* increase channel syncpoint threshold for MW_ACK_DONE */ > + buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1); > + > + /* Program MW_ACK_DONE trigger condition syncpt request */ > + mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno); > + value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) | > + host1x_syncpt_id(chan->mw_ack_sp); > + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); > + > + /* enable single shot capture */ > + vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE); > + chan->capture_reqs++; > + > + /* wait for syncpt counter to reach frame start event threshold */ > + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, > + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); > + if (err) { > + dev_err(&chan->video.dev, > + "frame start syncpt timeout: %d\n", err); > + /* increment syncpoint counter for timedout events */ > + host1x_syncpt_incr(chan->frame_start_sp); Why incrementing is done while hardware is still active? The sync point's state needs to be completely reset after resetting hardware. But I don't think that the current upstream host1x driver supports doing that, it's one of the known-long-standing problems of the host1x driver. At least the sp->max_val incrementing should be done based on the actual syncpoint value and this should be done after resetting hardware. > + spin_lock(&chan->sp_incr_lock); > + host1x_syncpt_incr(chan->mw_ack_sp); > + spin_unlock(&chan->sp_incr_lock); > + /* clear errors and recover */ > + tegra_channel_capture_error_recover(chan); > + release_buffer(chan, buf, VB2_BUF_STATE_ERROR); > + return err; > + }
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +config VIDEO_TEGRA > + tristate "NVIDIA Tegra VI driver" > + depends on ARCH_TEGRA || (ARM && COMPILE_TEST) Why COMPILE_TEST depends on ARM?
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi) > +{ > + struct tegra_vi_channel *chan, *tmp; > + unsigned int port_num; > + unsigned int nchannels = vi->soc->vi_max_channels; > + int ret = 0; > + > + for (port_num = 0; port_num < nchannels; port_num++) { > + /* > + * Do not use devm_kzalloc as memory is freed immediately > + * when device instance is unbound but application might still > + * be holding the device node open. Channel memory allocated > + * with kzalloc is freed during video device release callback. > + */ > + chan = kzalloc(sizeof(*chan), GFP_KERNEL); Why anyone would want to unbind this driver in practice? I think it should make more sense to set suppress_bind_attrs=true.
On 4/5/20 1:35 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 04.04.2020 04:25, Sowjanya Komatineni пишет: > ... >> +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan, >> + struct tegra_channel_buffer *buf) >> +{ >> + int err = 0; >> + u32 thresh, value, frame_start, mw_ack_done; >> + int bytes_per_line = chan->format.bytesperline; >> + >> + /* program buffer address by using surface 0 */ >> + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, >> + (u64)buf->addr >> 32); >> + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr); >> + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line); >> + >> + /* >> + * Tegra VI block interacts with host1x syncpt for synchronizing >> + * programmed condition of capture state and hardware operation. >> + * Frame start and Memory write acknowledge syncpts has their own >> + * FIFO of depth 2. >> + * >> + * Syncpoint trigger conditions set through VI_INCR_SYNCPT register >> + * are added to HW syncpt FIFO and when the HW triggers, syncpt >> + * condition is removed from the FIFO and counter at syncpoint index >> + * will be incremented by the hardware and software can wait for >> + * counter to reach threshold to synchronize capturing frame with the >> + * hardware capture events. >> + */ >> + >> + /* increase channel syncpoint threshold for FRAME_START */ >> + thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1); >> + >> + /* Program FRAME_START trigger condition syncpt request */ >> + frame_start = VI_CSI_PP_FRAME_START(chan->portno); >> + value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) | >> + host1x_syncpt_id(chan->frame_start_sp); >> + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); >> + >> + /* increase channel syncpoint threshold for MW_ACK_DONE */ >> + buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1); >> + >> + /* Program MW_ACK_DONE trigger condition syncpt request */ >> + mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno); >> + value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) | >> + host1x_syncpt_id(chan->mw_ack_sp); >> + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); >> + >> + /* enable single shot capture */ >> + vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE); >> + chan->capture_reqs++; >> + >> + /* wait for syncpt counter to reach frame start event threshold */ >> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >> + if (err) { >> + dev_err(&chan->video.dev, >> + "frame start syncpt timeout: %d\n", err); >> + /* increment syncpoint counter for timedout events */ >> + host1x_syncpt_incr(chan->frame_start_sp); > Why incrementing is done while hardware is still active? > > The sync point's state needs to be completely reset after resetting > hardware. But I don't think that the current upstream host1x driver > supports doing that, it's one of the known-long-standing problems of the > host1x driver. > > At least the sp->max_val incrementing should be done based on the actual > syncpoint value and this should be done after resetting hardware. upstream host1x driver don't have API to reset or to equalize max value with min/load value. So to synchronize missed event, incrementing HW syncpt counter. This should not impact as we increment this in case of missed events only. >> + spin_lock(&chan->sp_incr_lock); >> + host1x_syncpt_incr(chan->mw_ack_sp); >> + spin_unlock(&chan->sp_incr_lock); >> + /* clear errors and recover */ >> + tegra_channel_capture_error_recover(chan); >> + release_buffer(chan, buf, VB2_BUF_STATE_ERROR); >> + return err; >> + }
On 4/5/20 2:11 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 04.04.2020 04:25, Sowjanya Komatineni пишет: > ... >> +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi) >> +{ >> + struct tegra_vi_channel *chan, *tmp; >> + unsigned int port_num; >> + unsigned int nchannels = vi->soc->vi_max_channels; >> + int ret = 0; >> + >> + for (port_num = 0; port_num < nchannels; port_num++) { >> + /* >> + * Do not use devm_kzalloc as memory is freed immediately >> + * when device instance is unbound but application might still >> + * be holding the device node open. Channel memory allocated >> + * with kzalloc is freed during video device release callback. >> + */ >> + chan = kzalloc(sizeof(*chan), GFP_KERNEL); > Why anyone would want to unbind this driver in practice? > > I think it should make more sense to set suppress_bind_attrs=true. From the previous feedback of patch series, we need to support unbind/bind and looks like this driver should also support to built as a module.
06.04.2020 18:35, Sowjanya Komatineni пишет: ... >>> + /* wait for syncpt counter to reach frame start event threshold */ >>> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >>> + if (err) { >>> + dev_err(&chan->video.dev, >>> + "frame start syncpt timeout: %d\n", err); >>> + /* increment syncpoint counter for timedout events */ >>> + host1x_syncpt_incr(chan->frame_start_sp); >> Why incrementing is done while hardware is still active? >> >> The sync point's state needs to be completely reset after resetting >> hardware. But I don't think that the current upstream host1x driver >> supports doing that, it's one of the known-long-standing problems of the >> host1x driver. >> >> At least the sp->max_val incrementing should be done based on the actual >> syncpoint value and this should be done after resetting hardware. > > upstream host1x driver don't have API to reset or to equalize max value > with min/load value. > > So to synchronize missed event, incrementing HW syncpt counter. > > This should not impact as we increment this in case of missed events only. It's wrong to touch sync point while hardware is active and it's active until being reset. You should re-check the timeout after hw resetting and manually put the syncpoint counter back into sync only if needed.
06.04.2020 18:41, Sowjanya Komatineni пишет: > > On 4/5/20 2:11 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 04.04.2020 04:25, Sowjanya Komatineni пишет: >> ... >>> +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi) >>> +{ >>> + struct tegra_vi_channel *chan, *tmp; >>> + unsigned int port_num; >>> + unsigned int nchannels = vi->soc->vi_max_channels; >>> + int ret = 0; >>> + >>> + for (port_num = 0; port_num < nchannels; port_num++) { >>> + /* >>> + * Do not use devm_kzalloc as memory is freed immediately >>> + * when device instance is unbound but application >>> might still >>> + * be holding the device node open. Channel memory >>> allocated >>> + * with kzalloc is freed during video device release >>> callback. >>> + */ >>> + chan = kzalloc(sizeof(*chan), GFP_KERNEL); >> Why anyone would want to unbind this driver in practice? >> >> I think it should make more sense to set suppress_bind_attrs=true. > > From the previous feedback of patch series, we need to support > unbind/bind and looks like this driver should also support to built as a > module. If module unloading is also affected, then perhaps you should use get/put_device() to not allow freeing the resources until they're still in-use. I suppose that it should be up to the V4L core to keep the device alive while needed, rather than to put the burden to the individual drivers.
On 4/6/20 9:05 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 18:35, Sowjanya Komatineni пишет: > ... >>>> + /* wait for syncpt counter to reach frame start event threshold */ >>>> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >>>> + if (err) { >>>> + dev_err(&chan->video.dev, >>>> + "frame start syncpt timeout: %d\n", err); >>>> + /* increment syncpoint counter for timedout events */ >>>> + host1x_syncpt_incr(chan->frame_start_sp); >>> Why incrementing is done while hardware is still active? >>> >>> The sync point's state needs to be completely reset after resetting >>> hardware. But I don't think that the current upstream host1x driver >>> supports doing that, it's one of the known-long-standing problems of the >>> host1x driver. >>> >>> At least the sp->max_val incrementing should be done based on the actual >>> syncpoint value and this should be done after resetting hardware. >> upstream host1x driver don't have API to reset or to equalize max value >> with min/load value. >> >> So to synchronize missed event, incrementing HW syncpt counter. >> >> This should not impact as we increment this in case of missed events only. > It's wrong to touch sync point while hardware is active and it's active > until being reset. > > You should re-check the timeout after hw resetting and manually put the > syncpoint counter back into sync only if needed. There is possibility of timeout to happen any time even during the capture also and is not related to hw reset. Manual synchronization is needed when timeout of any frame events happen otherwise all subsequence frames will timeout due to mismatch in event counters.
06.04.2020 19:12, Sowjanya Komatineni пишет: > > On 4/6/20 9:05 AM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 06.04.2020 18:35, Sowjanya Komatineni пишет: >> ... >>>>> + /* wait for syncpt counter to reach frame start event >>>>> threshold */ >>>>> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >>>>> + if (err) { >>>>> + dev_err(&chan->video.dev, >>>>> + "frame start syncpt timeout: %d\n", err); >>>>> + /* increment syncpoint counter for timedout events */ >>>>> + host1x_syncpt_incr(chan->frame_start_sp); >>>> Why incrementing is done while hardware is still active? >>>> >>>> The sync point's state needs to be completely reset after resetting >>>> hardware. But I don't think that the current upstream host1x driver >>>> supports doing that, it's one of the known-long-standing problems of >>>> the >>>> host1x driver. >>>> >>>> At least the sp->max_val incrementing should be done based on the >>>> actual >>>> syncpoint value and this should be done after resetting hardware. >>> upstream host1x driver don't have API to reset or to equalize max value >>> with min/load value. >>> >>> So to synchronize missed event, incrementing HW syncpt counter. >>> >>> This should not impact as we increment this in case of missed events >>> only. >> It's wrong to touch sync point while hardware is active and it's active >> until being reset. >> >> You should re-check the timeout after hw resetting and manually put the >> syncpoint counter back into sync only if needed. > > There is possibility of timeout to happen any time even during the > capture also and is not related to hw reset. > > Manual synchronization is needed when timeout of any frame events happen > otherwise all subsequence frames will timeout due to mismatch in event > counters. My point is that hardware is stopped only after being reset, until then you should assume that sync point could be incremented by HW at any time. And if this happens that HW increments sync point after the timeout, then the sync point counter should become out-of-sync in yours case, IIUC. Because host1x_syncpt_incr() doesn't update the cached counter.
On 4/6/20 9:29 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 19:12, Sowjanya Komatineni пишет: >> On 4/6/20 9:05 AM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 06.04.2020 18:35, Sowjanya Komatineni пишет: >>> ... >>>>>> + /* wait for syncpt counter to reach frame start event >>>>>> threshold */ >>>>>> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >>>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >>>>>> + if (err) { >>>>>> + dev_err(&chan->video.dev, >>>>>> + "frame start syncpt timeout: %d\n", err); >>>>>> + /* increment syncpoint counter for timedout events */ >>>>>> + host1x_syncpt_incr(chan->frame_start_sp); >>>>> Why incrementing is done while hardware is still active? >>>>> >>>>> The sync point's state needs to be completely reset after resetting >>>>> hardware. But I don't think that the current upstream host1x driver >>>>> supports doing that, it's one of the known-long-standing problems of >>>>> the >>>>> host1x driver. >>>>> >>>>> At least the sp->max_val incrementing should be done based on the >>>>> actual >>>>> syncpoint value and this should be done after resetting hardware. >>>> upstream host1x driver don't have API to reset or to equalize max value >>>> with min/load value. >>>> >>>> So to synchronize missed event, incrementing HW syncpt counter. >>>> >>>> This should not impact as we increment this in case of missed events >>>> only. >>> It's wrong to touch sync point while hardware is active and it's active >>> until being reset. >>> >>> You should re-check the timeout after hw resetting and manually put the >>> syncpoint counter back into sync only if needed. >> There is possibility of timeout to happen any time even during the >> capture also and is not related to hw reset. >> >> Manual synchronization is needed when timeout of any frame events happen >> otherwise all subsequence frames will timeout due to mismatch in event >> counters. > My point is that hardware is stopped only after being reset, until then > you should assume that sync point could be incremented by HW at any time. > > And if this happens that HW increments sync point after the timeout, > then the sync point counter should become out-of-sync in yours case, > IIUC. Because host1x_syncpt_incr() doesn't update the cached counter. We wait for enough time based on frame rate for syncpt increment to happen and if it doesn't happen by then definitely its missed event and we increment HW syncpoint for this timed event. cached value gets updated during syncpt wait for subsequent event. syncpt increment happens for all subsequent frame events during video capture.
On 4/6/20 9:37 AM, Sowjanya Komatineni wrote: > > On 4/6/20 9:29 AM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 06.04.2020 19:12, Sowjanya Komatineni пишет: >>> On 4/6/20 9:05 AM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 06.04.2020 18:35, Sowjanya Komatineni пишет: >>>> ... >>>>>>> + /* wait for syncpt counter to reach frame start event >>>>>>> threshold */ >>>>>>> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >>>>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >>>>>>> + if (err) { >>>>>>> + dev_err(&chan->video.dev, >>>>>>> + "frame start syncpt timeout: %d\n", err); >>>>>>> + /* increment syncpoint counter for timedout events */ >>>>>>> + host1x_syncpt_incr(chan->frame_start_sp); >>>>>> Why incrementing is done while hardware is still active? >>>>>> >>>>>> The sync point's state needs to be completely reset after resetting >>>>>> hardware. But I don't think that the current upstream host1x driver >>>>>> supports doing that, it's one of the known-long-standing problems of >>>>>> the >>>>>> host1x driver. >>>>>> >>>>>> At least the sp->max_val incrementing should be done based on the >>>>>> actual >>>>>> syncpoint value and this should be done after resetting hardware. >>>>> upstream host1x driver don't have API to reset or to equalize max >>>>> value >>>>> with min/load value. >>>>> >>>>> So to synchronize missed event, incrementing HW syncpt counter. >>>>> >>>>> This should not impact as we increment this in case of missed events >>>>> only. >>>> It's wrong to touch sync point while hardware is active and it's >>>> active >>>> until being reset. >>>> >>>> You should re-check the timeout after hw resetting and manually put >>>> the >>>> syncpoint counter back into sync only if needed. >>> There is possibility of timeout to happen any time even during the >>> capture also and is not related to hw reset. >>> >>> Manual synchronization is needed when timeout of any frame events >>> happen >>> otherwise all subsequence frames will timeout due to mismatch in event >>> counters. >> My point is that hardware is stopped only after being reset, until then >> you should assume that sync point could be incremented by HW at any >> time. >> >> And if this happens that HW increments sync point after the timeout, >> then the sync point counter should become out-of-sync in yours case, >> IIUC. Because host1x_syncpt_incr() doesn't update the cached counter. > > We wait for enough time based on frame rate for syncpt increment to > happen and if it doesn't happen by then definitely its missed event > and we increment HW syncpoint for this timed event. > > cached value gets updated during syncpt wait for subsequent event. > > syncpt increment happens for all subsequent frame events during video > capture. > Just to be clear, syncpt max value increment happens first and syncpt trigger condition is programmed. hw syncpt increment happens based on HW events. Wait time for HW syncpt to reach threshold is tuned to work for all frame rates. So if increment doesn't happen by then, its definitely missed event. In case of missed HW event corresponding to syncpt condition, hw syncpt increment does not happen and driver increments it on timeout. As there is not API to equialize max with min incase of timeout/reset, incrementing HW syncpt for timed out event. syncpt cached value gets updated during syncpt wait when it loads from HW syncpt. As syncpt condition is already triggered, without compensating timeout events or leaving syncpt max and hw syncpt in non synchronized state for missed events, subsequent streamings will all timeout even on real events.
On 4/5/20 12:37 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 04.04.2020 04:25, Sowjanya Komatineni пишет: > ... >> +static int tegra_csi_init(struct host1x_client *client) >> +{ >> + struct tegra_csi *csi = host1x_client_to_csi(client); >> + struct tegra_video_device *vid = dev_get_drvdata(client->host); >> + int ret; >> + >> + vid->csi = csi; >> + >> + INIT_LIST_HEAD(&csi->csi_chans); >> + >> + if (pm_runtime_enabled(csi->dev)) { >> + 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; >> + } >> + } else { > RPM is supposed to be always available on Tegra nowadays. Sorry I was not sure if its all the time enabled, so added in v6. Will remove check and explicit runtime calls...
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan, > + struct tegra_channel_buffer *buf) > +{ > + int err = 0; > + u32 thresh, value, frame_start, mw_ack_done; > + int bytes_per_line = chan->format.bytesperline; > + > + /* program buffer address by using surface 0 */ > + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, > + (u64)buf->addr >> 32); > + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr); > + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line); > + > + /* > + * Tegra VI block interacts with host1x syncpt for synchronizing > + * programmed condition of capture state and hardware operation. > + * Frame start and Memory write acknowledge syncpts has their own > + * FIFO of depth 2. > + * > + * Syncpoint trigger conditions set through VI_INCR_SYNCPT register > + * are added to HW syncpt FIFO and when the HW triggers, syncpt > + * condition is removed from the FIFO and counter at syncpoint index > + * will be incremented by the hardware and software can wait for > + * counter to reach threshold to synchronize capturing frame with the > + * hardware capture events. > + */ > + > + /* increase channel syncpoint threshold for FRAME_START */ > + thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1); > + > + /* Program FRAME_START trigger condition syncpt request */ > + frame_start = VI_CSI_PP_FRAME_START(chan->portno); > + value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) | > + host1x_syncpt_id(chan->frame_start_sp); > + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); > + > + /* increase channel syncpoint threshold for MW_ACK_DONE */ > + buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1); > + > + /* Program MW_ACK_DONE trigger condition syncpt request */ > + mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno); > + value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) | > + host1x_syncpt_id(chan->mw_ack_sp); > + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); > + > + /* enable single shot capture */ > + vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE); > + chan->capture_reqs++; > + > + /* wait for syncpt counter to reach frame start event threshold */ > + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, > + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); What is the point of waiting for the frame-start? Why just not to wait for the frame-end?
06.04.2020 20:02, Sowjanya Komatineni пишет: > > On 4/6/20 9:37 AM, Sowjanya Komatineni wrote: >> >> On 4/6/20 9:29 AM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 06.04.2020 19:12, Sowjanya Komatineni пишет: >>>> On 4/6/20 9:05 AM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 06.04.2020 18:35, Sowjanya Komatineni пишет: >>>>> ... >>>>>>>> + /* wait for syncpt counter to reach frame start event >>>>>>>> threshold */ >>>>>>>> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >>>>>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >>>>>>>> + if (err) { >>>>>>>> + dev_err(&chan->video.dev, >>>>>>>> + "frame start syncpt timeout: %d\n", err); >>>>>>>> + /* increment syncpoint counter for timedout events */ >>>>>>>> + host1x_syncpt_incr(chan->frame_start_sp); >>>>>>> Why incrementing is done while hardware is still active? >>>>>>> >>>>>>> The sync point's state needs to be completely reset after resetting >>>>>>> hardware. But I don't think that the current upstream host1x driver >>>>>>> supports doing that, it's one of the known-long-standing problems of >>>>>>> the >>>>>>> host1x driver. >>>>>>> >>>>>>> At least the sp->max_val incrementing should be done based on the >>>>>>> actual >>>>>>> syncpoint value and this should be done after resetting hardware. >>>>>> upstream host1x driver don't have API to reset or to equalize max >>>>>> value >>>>>> with min/load value. >>>>>> >>>>>> So to synchronize missed event, incrementing HW syncpt counter. >>>>>> >>>>>> This should not impact as we increment this in case of missed events >>>>>> only. >>>>> It's wrong to touch sync point while hardware is active and it's >>>>> active >>>>> until being reset. >>>>> >>>>> You should re-check the timeout after hw resetting and manually put >>>>> the >>>>> syncpoint counter back into sync only if needed. >>>> There is possibility of timeout to happen any time even during the >>>> capture also and is not related to hw reset. >>>> >>>> Manual synchronization is needed when timeout of any frame events >>>> happen >>>> otherwise all subsequence frames will timeout due to mismatch in event >>>> counters. >>> My point is that hardware is stopped only after being reset, until then >>> you should assume that sync point could be incremented by HW at any >>> time. >>> >>> And if this happens that HW increments sync point after the timeout, >>> then the sync point counter should become out-of-sync in yours case, >>> IIUC. Because host1x_syncpt_incr() doesn't update the cached counter. >> >> We wait for enough time based on frame rate for syncpt increment to >> happen and if it doesn't happen by then definitely its missed event >> and we increment HW syncpoint for this timed event. >> >> cached value gets updated during syncpt wait for subsequent event. >> >> syncpt increment happens for all subsequent frame events during video >> capture. >> > Just to be clear, syncpt max value increment happens first and syncpt > trigger condition is programmed. hw syncpt increment happens based on HW > events. > > Wait time for HW syncpt to reach threshold is tuned to work for all > frame rates. So if increment doesn't happen by then, its definitely > missed event. This is questionable. Technically, speculating about whether the tuned value is good for all possible cases is incorrect thing to do. Although, I guess in practice it should be good enough for the starter and could be improved later on, once the host1x driver will be improved. > In case of missed HW event corresponding to syncpt condition, hw syncpt > increment does not happen and driver increments it on timeout. > > As there is not API to equialize max with min incase of timeout/reset, > incrementing HW syncpt for timed out event. > > syncpt cached value gets updated during syncpt wait when it loads from > HW syncpt. > > As syncpt condition is already triggered, without compensating timeout > events or leaving syncpt max and hw syncpt in non synchronized state for > missed events, subsequent streamings will all timeout even on real events. >
On 4/6/20 12:48 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 04.04.2020 04:25, Sowjanya Komatineni пишет: > ... >> +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan, >> + struct tegra_channel_buffer *buf) >> +{ >> + int err = 0; >> + u32 thresh, value, frame_start, mw_ack_done; >> + int bytes_per_line = chan->format.bytesperline; >> + >> + /* program buffer address by using surface 0 */ >> + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, >> + (u64)buf->addr >> 32); >> + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr); >> + vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line); >> + >> + /* >> + * Tegra VI block interacts with host1x syncpt for synchronizing >> + * programmed condition of capture state and hardware operation. >> + * Frame start and Memory write acknowledge syncpts has their own >> + * FIFO of depth 2. >> + * >> + * Syncpoint trigger conditions set through VI_INCR_SYNCPT register >> + * are added to HW syncpt FIFO and when the HW triggers, syncpt >> + * condition is removed from the FIFO and counter at syncpoint index >> + * will be incremented by the hardware and software can wait for >> + * counter to reach threshold to synchronize capturing frame with the >> + * hardware capture events. >> + */ >> + >> + /* increase channel syncpoint threshold for FRAME_START */ >> + thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1); >> + >> + /* Program FRAME_START trigger condition syncpt request */ >> + frame_start = VI_CSI_PP_FRAME_START(chan->portno); >> + value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) | >> + host1x_syncpt_id(chan->frame_start_sp); >> + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); >> + >> + /* increase channel syncpoint threshold for MW_ACK_DONE */ >> + buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1); >> + >> + /* Program MW_ACK_DONE trigger condition syncpt request */ >> + mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno); >> + value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) | >> + host1x_syncpt_id(chan->mw_ack_sp); >> + tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value); >> + >> + /* enable single shot capture */ >> + vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE); >> + chan->capture_reqs++; >> + >> + /* wait for syncpt counter to reach frame start event threshold */ >> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); > What is the point of waiting for the frame-start? Why just not to wait > for the frame-end? Tegra vi supports double buffering where up on receiving frame start before HW received frame end and finish writing capture data to memory, we can issue next frame as well a head. Also MW_ACK timeout can happen incase of HDMI2CSI bridges as well when hdmi hot plug happens. For some sensors down the road we may need to skip few frames in case of frame start timeout as well which comes later with subsequent patch series.
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +static int chan_capture_kthread_start(void *data) > +{ > + struct tegra_vi_channel *chan = data; > + struct tegra_channel_buffer *buf; > + int err = 0; > + int caps_inflight; > + > + set_freezable(); > + > + while (1) { > + try_to_freeze(); > + > + wait_event_interruptible(chan->start_wait, > + !list_empty(&chan->capture) || > + kthread_should_stop()); Is it really okay that list_empty() isn't protected with a lock? Why wait_event is "interruptible"? > + /* > + * Frame start and MW_ACK_DONE syncpoint condition FIFOs are > + * of max depth 2. So make sure max 2 capture requests are > + * in process by the hardware at a time. > + */ > + while (!(kthread_should_stop() || list_empty(&chan->capture))) { > + caps_inflight = chan->capture_reqs - chan->sequence; > + /* > + * Source is not streaming if error is non-zero. > + * So, do not dequeue buffers on capture error or when > + * syncpoint requests in FIFO are full. > + */ > + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) > + break; > + > + /* dequeue the buffer and start capture */ > + spin_lock(&chan->start_lock); > + if (list_empty(&chan->capture)) > + break; > + buf = list_entry(chan->capture.next, > + struct tegra_channel_buffer, queue); list_first_entry() > + list_del_init(&buf->queue); > + spin_unlock(&chan->start_lock); > + > + err = tegra_channel_capture_frame(chan, buf); > + if (err) > + vb2_queue_error(&chan->queue); > + } > + > + if (kthread_should_stop()) > + break; > + } > + > + return 0; > +}
On 4/6/20 12:53 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 20:02, Sowjanya Komatineni пишет: >> On 4/6/20 9:37 AM, Sowjanya Komatineni wrote: >>> On 4/6/20 9:29 AM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 06.04.2020 19:12, Sowjanya Komatineni пишет: >>>>> On 4/6/20 9:05 AM, Dmitry Osipenko wrote: >>>>>> External email: Use caution opening links or attachments >>>>>> >>>>>> >>>>>> 06.04.2020 18:35, Sowjanya Komatineni пишет: >>>>>> ... >>>>>>>>> + /* wait for syncpt counter to reach frame start event >>>>>>>>> threshold */ >>>>>>>>> + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, >>>>>>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); >>>>>>>>> + if (err) { >>>>>>>>> + dev_err(&chan->video.dev, >>>>>>>>> + "frame start syncpt timeout: %d\n", err); >>>>>>>>> + /* increment syncpoint counter for timedout events */ >>>>>>>>> + host1x_syncpt_incr(chan->frame_start_sp); >>>>>>>> Why incrementing is done while hardware is still active? >>>>>>>> >>>>>>>> The sync point's state needs to be completely reset after resetting >>>>>>>> hardware. But I don't think that the current upstream host1x driver >>>>>>>> supports doing that, it's one of the known-long-standing problems of >>>>>>>> the >>>>>>>> host1x driver. >>>>>>>> >>>>>>>> At least the sp->max_val incrementing should be done based on the >>>>>>>> actual >>>>>>>> syncpoint value and this should be done after resetting hardware. >>>>>>> upstream host1x driver don't have API to reset or to equalize max >>>>>>> value >>>>>>> with min/load value. >>>>>>> >>>>>>> So to synchronize missed event, incrementing HW syncpt counter. >>>>>>> >>>>>>> This should not impact as we increment this in case of missed events >>>>>>> only. >>>>>> It's wrong to touch sync point while hardware is active and it's >>>>>> active >>>>>> until being reset. >>>>>> >>>>>> You should re-check the timeout after hw resetting and manually put >>>>>> the >>>>>> syncpoint counter back into sync only if needed. >>>>> There is possibility of timeout to happen any time even during the >>>>> capture also and is not related to hw reset. >>>>> >>>>> Manual synchronization is needed when timeout of any frame events >>>>> happen >>>>> otherwise all subsequence frames will timeout due to mismatch in event >>>>> counters. >>>> My point is that hardware is stopped only after being reset, until then >>>> you should assume that sync point could be incremented by HW at any >>>> time. >>>> >>>> And if this happens that HW increments sync point after the timeout, >>>> then the sync point counter should become out-of-sync in yours case, >>>> IIUC. Because host1x_syncpt_incr() doesn't update the cached counter. >>> We wait for enough time based on frame rate for syncpt increment to >>> happen and if it doesn't happen by then definitely its missed event >>> and we increment HW syncpoint for this timed event. >>> >>> cached value gets updated during syncpt wait for subsequent event. >>> >>> syncpt increment happens for all subsequent frame events during video >>> capture. >>> >> Just to be clear, syncpt max value increment happens first and syncpt >> trigger condition is programmed. hw syncpt increment happens based on HW >> events. >> >> Wait time for HW syncpt to reach threshold is tuned to work for all >> frame rates. So if increment doesn't happen by then, its definitely >> missed event. > This is questionable. Technically, speculating about whether the tuned > value is good for all possible cases is incorrect thing to do. > > Although, I guess in practice it should be good enough for the starter > and could be improved later on, once the host1x driver will be improved. By tuned value I meant about 200ms wait timeout for frame event to happen is what we have been using in downstream and with BSP release images which works good for all sensors and bridges we supported so far. > >> In case of missed HW event corresponding to syncpt condition, hw syncpt >> increment does not happen and driver increments it on timeout. >> >> As there is not API to equialize max with min incase of timeout/reset, >> incrementing HW syncpt for timed out event. >> >> syncpt cached value gets updated during syncpt wait when it loads from >> HW syncpt. >> >> As syncpt condition is already triggered, without compensating timeout >> events or leaving syncpt max and hw syncpt in non synchronized state for >> missed events, subsequent streamings will all timeout even on real events. >>
On 4/6/20 1:02 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 04.04.2020 04:25, Sowjanya Komatineni пишет: > ... >> +static int chan_capture_kthread_start(void *data) >> +{ >> + struct tegra_vi_channel *chan = data; >> + struct tegra_channel_buffer *buf; >> + int err = 0; >> + int caps_inflight; >> + >> + set_freezable(); >> + >> + while (1) { >> + try_to_freeze(); >> + >> + wait_event_interruptible(chan->start_wait, >> + !list_empty(&chan->capture) || >> + kthread_should_stop()); > Is it really okay that list_empty() isn't protected with a lock? > > Why wait_event is "interruptible"? To allow it to sleep until wakeup on thread it to avoid constant checking for condition even when no buffers are ready, basically to prevent blocking. > >> + /* >> + * Frame start and MW_ACK_DONE syncpoint condition FIFOs are >> + * of max depth 2. So make sure max 2 capture requests are >> + * in process by the hardware at a time. >> + */ >> + while (!(kthread_should_stop() || list_empty(&chan->capture))) { >> + caps_inflight = chan->capture_reqs - chan->sequence; >> + /* >> + * Source is not streaming if error is non-zero. >> + * So, do not dequeue buffers on capture error or when >> + * syncpoint requests in FIFO are full. >> + */ >> + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) >> + break; >> + >> + /* dequeue the buffer and start capture */ >> + spin_lock(&chan->start_lock); >> + if (list_empty(&chan->capture)) >> + break; >> + buf = list_entry(chan->capture.next, >> + struct tegra_channel_buffer, queue); > list_first_entry() > >> + list_del_init(&buf->queue); >> + spin_unlock(&chan->start_lock); >> + >> + err = tegra_channel_capture_frame(chan, buf); >> + if (err) >> + vb2_queue_error(&chan->queue); >> + } >> + >> + if (kthread_should_stop()) >> + break; >> + } >> + >> + return 0; >> +}
06.04.2020 23:05, Sowjanya Komatineni пишет: .. >>> Wait time for HW syncpt to reach threshold is tuned to work for all >>> frame rates. So if increment doesn't happen by then, its definitely >>> missed event. >> This is questionable. Technically, speculating about whether the tuned >> value is good for all possible cases is incorrect thing to do. >> >> Although, I guess in practice it should be good enough for the starter >> and could be improved later on, once the host1x driver will be improved. > > By tuned value I meant about 200ms wait timeout for frame event to > happen is what we have been using in downstream and with BSP release > images which works good for all sensors and bridges we supported so far. I don't know anything about the state of today's downstream, but downstream of older Tegra SoCs was pretty awful in regards to the host1x syncing, unfortunately it was borrowed into the upstream host1x years ago and nothing was done about it so far. I'd suggest to be careful about it.
On 4/6/20 1:28 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 23:05, Sowjanya Komatineni пишет: > .. >>>> Wait time for HW syncpt to reach threshold is tuned to work for all >>>> frame rates. So if increment doesn't happen by then, its definitely >>>> missed event. >>> This is questionable. Technically, speculating about whether the tuned >>> value is good for all possible cases is incorrect thing to do. >>> >>> Although, I guess in practice it should be good enough for the starter >>> and could be improved later on, once the host1x driver will be improved. >> By tuned value I meant about 200ms wait timeout for frame event to >> happen is what we have been using in downstream and with BSP release >> images which works good for all sensors and bridges we supported so far. > I don't know anything about the state of today's downstream, but > downstream of older Tegra SoCs was pretty awful in regards to the host1x > syncing, unfortunately it was borrowed into the upstream host1x years > ago and nothing was done about it so far. I'd suggest to be careful > about it. 200ms timeout we wait for event to happen is the case even with T186/T194 as well and internally it was tuned from lots of testing with various sensors and frame rate computations which is known to work good.
06.04.2020 23:20, Sowjanya Komatineni пишет: > > On 4/6/20 1:02 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 04.04.2020 04:25, Sowjanya Komatineni пишет: >> ... >>> +static int chan_capture_kthread_start(void *data) >>> +{ >>> + struct tegra_vi_channel *chan = data; >>> + struct tegra_channel_buffer *buf; >>> + int err = 0; >>> + int caps_inflight; >>> + >>> + set_freezable(); >>> + >>> + while (1) { >>> + try_to_freeze(); >>> + >>> + wait_event_interruptible(chan->start_wait, >>> + !list_empty(&chan->capture) || >>> + kthread_should_stop()); >> Is it really okay that list_empty() isn't protected with a lock? >> >> Why wait_event is "interruptible"? > > To allow it to sleep until wakeup on thread it to avoid constant > checking for condition even when no buffers are ready, basically to > prevent blocking. So the "interrupt" is for getting event about kthread_should_stop(), correct?
On 4/6/20 1:37 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 23:20, Sowjanya Komatineni пишет: >> On 4/6/20 1:02 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>> ... >>>> +static int chan_capture_kthread_start(void *data) >>>> +{ >>>> + struct tegra_vi_channel *chan = data; >>>> + struct tegra_channel_buffer *buf; >>>> + int err = 0; >>>> + int caps_inflight; >>>> + >>>> + set_freezable(); >>>> + >>>> + while (1) { >>>> + try_to_freeze(); >>>> + >>>> + wait_event_interruptible(chan->start_wait, >>>> + !list_empty(&chan->capture) || >>>> + kthread_should_stop()); >>> Is it really okay that list_empty() isn't protected with a lock? >>> >>> Why wait_event is "interruptible"? >> To allow it to sleep until wakeup on thread it to avoid constant >> checking for condition even when no buffers are ready, basically to >> prevent blocking. > So the "interrupt" is for getting event about kthread_should_stop(), > correct? also to prevent blocking and to let is sleep and wakeup based on wait queue to evaluate condition to proceed with the task >
On 4/6/20 1:38 PM, Sowjanya Komatineni wrote: > > On 4/6/20 1:37 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 06.04.2020 23:20, Sowjanya Komatineni пишет: >>> On 4/6/20 1:02 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>>> ... >>>>> +static int chan_capture_kthread_start(void *data) >>>>> +{ >>>>> + struct tegra_vi_channel *chan = data; >>>>> + struct tegra_channel_buffer *buf; >>>>> + int err = 0; >>>>> + int caps_inflight; >>>>> + >>>>> + set_freezable(); >>>>> + >>>>> + while (1) { >>>>> + try_to_freeze(); >>>>> + >>>>> + wait_event_interruptible(chan->start_wait, >>>>> + !list_empty(&chan->capture) || >>>>> + kthread_should_stop()); >>>> Is it really okay that list_empty() isn't protected with a lock? wakeup on thread happens either when buffer is moved to capture list or on stop signaling event. So in this specific case we may not need to check for lock on capture list as if wakeup happens from start wait queue, then buffer is already moved to capture list by then. >>>> >>>> Why wait_event is "interruptible"? >>> To allow it to sleep until wakeup on thread it to avoid constant >>> checking for condition even when no buffers are ready, basically to >>> prevent blocking. >> So the "interrupt" is for getting event about kthread_should_stop(), >> correct? > also to prevent blocking and to let is sleep and wakeup based on wait > queue to evaluate condition to proceed with the task >>
04.04.2020 04:25, Sowjanya Komatineni пишет: > +static int chan_capture_kthread_start(void *data) > +{ > + struct tegra_vi_channel *chan = data; > + struct tegra_channel_buffer *buf; > + int err = 0; > + int caps_inflight; > + > + set_freezable(); > + > + while (1) { > + try_to_freeze(); > + > + wait_event_interruptible(chan->start_wait, > + !list_empty(&chan->capture) || > + kthread_should_stop()); > + /* > + * Frame start and MW_ACK_DONE syncpoint condition FIFOs are > + * of max depth 2. So make sure max 2 capture requests are > + * in process by the hardware at a time. > + */ > + while (!(kthread_should_stop() || list_empty(&chan->capture))) { > + caps_inflight = chan->capture_reqs - chan->sequence; > + /* > + * Source is not streaming if error is non-zero. > + * So, do not dequeue buffers on capture error or when > + * syncpoint requests in FIFO are full. > + */ > + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) > + break; Am I understanding correctly that this thread will take 100% CPU, spinning here, if more than 2 frame-captures queued?
On 4/6/20 1:45 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 04.04.2020 04:25, Sowjanya Komatineni пишет: >> +static int chan_capture_kthread_start(void *data) >> +{ >> + struct tegra_vi_channel *chan = data; >> + struct tegra_channel_buffer *buf; >> + int err = 0; >> + int caps_inflight; >> + >> + set_freezable(); >> + >> + while (1) { >> + try_to_freeze(); >> + >> + wait_event_interruptible(chan->start_wait, >> + !list_empty(&chan->capture) || >> + kthread_should_stop()); >> + /* >> + * Frame start and MW_ACK_DONE syncpoint condition FIFOs are >> + * of max depth 2. So make sure max 2 capture requests are >> + * in process by the hardware at a time. >> + */ >> + while (!(kthread_should_stop() || list_empty(&chan->capture))) { >> + caps_inflight = chan->capture_reqs - chan->sequence; >> + /* >> + * Source is not streaming if error is non-zero. >> + * So, do not dequeue buffers on capture error or when >> + * syncpoint requests in FIFO are full. >> + */ >> + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) >> + break; > Am I understanding correctly that this thread will take 100% CPU, > spinning here, if more than 2 frame-captures queued? on more than 2 frames captures, it breaks thread and on next wakeup it continues
06.04.2020 23:50, Sowjanya Komatineni пишет: > > On 4/6/20 1:45 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>> +static int chan_capture_kthread_start(void *data) >>> +{ >>> + struct tegra_vi_channel *chan = data; >>> + struct tegra_channel_buffer *buf; >>> + int err = 0; >>> + int caps_inflight; >>> + >>> + set_freezable(); >>> + >>> + while (1) { >>> + try_to_freeze(); >>> + >>> + wait_event_interruptible(chan->start_wait, >>> + !list_empty(&chan->capture) || >>> + kthread_should_stop()); >>> + /* >>> + * Frame start and MW_ACK_DONE syncpoint condition >>> FIFOs are >>> + * of max depth 2. So make sure max 2 capture requests are >>> + * in process by the hardware at a time. >>> + */ >>> + while (!(kthread_should_stop() || >>> list_empty(&chan->capture))) { >>> + caps_inflight = chan->capture_reqs - >>> chan->sequence; >>> + /* >>> + * Source is not streaming if error is non-zero. >>> + * So, do not dequeue buffers on capture error >>> or when >>> + * syncpoint requests in FIFO are full. >>> + */ >>> + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) >>> + break; >> Am I understanding correctly that this thread will take 100% CPU, >> spinning here, if more than 2 frame-captures queued? > on more than 2 frames captures, it breaks thread and on next wakeup it > continues The wait_event() won't wait if condition is true.
06.04.2020 23:38, Sowjanya Komatineni пишет: > > On 4/6/20 1:37 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 06.04.2020 23:20, Sowjanya Komatineni пишет: >>> On 4/6/20 1:02 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>>> ... >>>>> +static int chan_capture_kthread_start(void *data) >>>>> +{ >>>>> + struct tegra_vi_channel *chan = data; >>>>> + struct tegra_channel_buffer *buf; >>>>> + int err = 0; >>>>> + int caps_inflight; >>>>> + >>>>> + set_freezable(); >>>>> + >>>>> + while (1) { >>>>> + try_to_freeze(); >>>>> + >>>>> + wait_event_interruptible(chan->start_wait, >>>>> + !list_empty(&chan->capture) || >>>>> + kthread_should_stop()); >>>> Is it really okay that list_empty() isn't protected with a lock? >>>> >>>> Why wait_event is "interruptible"? >>> To allow it to sleep until wakeup on thread it to avoid constant >>> checking for condition even when no buffers are ready, basically to >>> prevent blocking. >> So the "interrupt" is for getting event about kthread_should_stop(), >> correct? > also to prevent blocking and to let is sleep and wakeup based on wait > queue to evaluate condition to proceed with the task >> This looks suspicious, the comment to wait_event_interruptible() says that it will return ERESTARTSYS if signal is recieved.. Does this mean that I can send signal from userspace to wake it up? The "interruptible" part looks wrong to me.
On 4/6/20 1:53 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 23:50, Sowjanya Komatineni пишет: >> On 4/6/20 1:45 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>>> +static int chan_capture_kthread_start(void *data) >>>> +{ >>>> + struct tegra_vi_channel *chan = data; >>>> + struct tegra_channel_buffer *buf; >>>> + int err = 0; >>>> + int caps_inflight; >>>> + >>>> + set_freezable(); >>>> + >>>> + while (1) { >>>> + try_to_freeze(); >>>> + >>>> + wait_event_interruptible(chan->start_wait, >>>> + !list_empty(&chan->capture) || >>>> + kthread_should_stop()); >>>> + /* >>>> + * Frame start and MW_ACK_DONE syncpoint condition >>>> FIFOs are >>>> + * of max depth 2. So make sure max 2 capture requests are >>>> + * in process by the hardware at a time. >>>> + */ >>>> + while (!(kthread_should_stop() || >>>> list_empty(&chan->capture))) { >>>> + caps_inflight = chan->capture_reqs - >>>> chan->sequence; >>>> + /* >>>> + * Source is not streaming if error is non-zero. >>>> + * So, do not dequeue buffers on capture error >>>> or when >>>> + * syncpoint requests in FIFO are full. >>>> + */ >>>> + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) >>>> + break; >>> Am I understanding correctly that this thread will take 100% CPU, >>> spinning here, if more than 2 frame-captures queued? >> on more than 2 frames captures, it breaks thread and on next wakeup it >> continues > The wait_event() won't wait if condition is true. condition is checked when waitqueue is woken up
06.04.2020 23:55, Sowjanya Komatineni пишет: > > On 4/6/20 1:53 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 06.04.2020 23:50, Sowjanya Komatineni пишет: >>> On 4/6/20 1:45 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>>>> +static int chan_capture_kthread_start(void *data) >>>>> +{ >>>>> + struct tegra_vi_channel *chan = data; >>>>> + struct tegra_channel_buffer *buf; >>>>> + int err = 0; >>>>> + int caps_inflight; >>>>> + >>>>> + set_freezable(); >>>>> + >>>>> + while (1) { >>>>> + try_to_freeze(); >>>>> + >>>>> + wait_event_interruptible(chan->start_wait, >>>>> + !list_empty(&chan->capture) || >>>>> + kthread_should_stop()); >>>>> + /* >>>>> + * Frame start and MW_ACK_DONE syncpoint condition >>>>> FIFOs are >>>>> + * of max depth 2. So make sure max 2 capture >>>>> requests are >>>>> + * in process by the hardware at a time. >>>>> + */ >>>>> + while (!(kthread_should_stop() || >>>>> list_empty(&chan->capture))) { >>>>> + caps_inflight = chan->capture_reqs - >>>>> chan->sequence; >>>>> + /* >>>>> + * Source is not streaming if error is non-zero. >>>>> + * So, do not dequeue buffers on capture error >>>>> or when >>>>> + * syncpoint requests in FIFO are full. >>>>> + */ >>>>> + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) >>>>> + break; >>>> Am I understanding correctly that this thread will take 100% CPU, >>>> spinning here, if more than 2 frame-captures queued? >>> on more than 2 frames captures, it breaks thread and on next wakeup it >>> continues >> The wait_event() won't wait if condition is true. > condition is checked when waitqueue is woken up https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462
On 4/6/20 1:56 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 23:55, Sowjanya Komatineni пишет: >> On 4/6/20 1:53 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 06.04.2020 23:50, Sowjanya Komatineni пишет: >>>> On 4/6/20 1:45 PM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>>>>> +static int chan_capture_kthread_start(void *data) >>>>>> +{ >>>>>> + struct tegra_vi_channel *chan = data; >>>>>> + struct tegra_channel_buffer *buf; >>>>>> + int err = 0; >>>>>> + int caps_inflight; >>>>>> + >>>>>> + set_freezable(); >>>>>> + >>>>>> + while (1) { >>>>>> + try_to_freeze(); >>>>>> + >>>>>> + wait_event_interruptible(chan->start_wait, >>>>>> + !list_empty(&chan->capture) || >>>>>> + kthread_should_stop()); >>>>>> + /* >>>>>> + * Frame start and MW_ACK_DONE syncpoint condition >>>>>> FIFOs are >>>>>> + * of max depth 2. So make sure max 2 capture >>>>>> requests are >>>>>> + * in process by the hardware at a time. >>>>>> + */ >>>>>> + while (!(kthread_should_stop() || >>>>>> list_empty(&chan->capture))) { >>>>>> + caps_inflight = chan->capture_reqs - >>>>>> chan->sequence; >>>>>> + /* >>>>>> + * Source is not streaming if error is non-zero. >>>>>> + * So, do not dequeue buffers on capture error >>>>>> or when >>>>>> + * syncpoint requests in FIFO are full. >>>>>> + */ >>>>>> + if (err || caps_inflight >= SYNCPT_FIFO_DEPTH) >>>>>> + break; >>>>> Am I understanding correctly that this thread will take 100% CPU, >>>>> spinning here, if more than 2 frame-captures queued? >>>> on more than 2 frames captures, it breaks thread and on next wakeup it >>>> continues >>> The wait_event() won't wait if condition is true. >> condition is checked when waitqueue is woken up > https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 process is put to sleep until the condition evaluates to true or signal is received. condition is checked each time the waitqueue head is woken up. Also capture list may keep on getting updated with buffers from userspace. but at a time we only limit 2 frames as VI supports double buffering and syncpt fifo's max depth is 2 Any more buffers waiting will be processing on subsequent iterations. So basically thread run time is depending on buffers getting queued from userspace.
07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>> Am I understanding correctly that this thread will take 100% CPU, >>>>>> spinning here, if more than 2 frame-captures queued? >>>>> on more than 2 frames captures, it breaks thread and on next wakeup it >>>>> continues >>>> The wait_event() won't wait if condition is true. >>> condition is checked when waitqueue is woken up >> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 > > process is put to sleep until the condition evaluates to true or signal > is received. > > condition is checked each time the waitqueue head is woken up. This is a wrong assumption in accordance to the code.
On 4/6/20 2:11 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>> Am I understanding correctly that this thread will take 100% CPU, >>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>> on more than 2 frames captures, it breaks thread and on next wakeup it >>>>>> continues >>>>> The wait_event() won't wait if condition is true. >>>> condition is checked when waitqueue is woken up >>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >> process is put to sleep until the condition evaluates to true or signal >> is received. >> >> condition is checked each time the waitqueue head is woken up. > This is a wrong assumption in accordance to the code. when every buffer is available as long as we are in streaming, we should process it. So if wake up happens when list has buffer, it will be processed but at a time we limit processing 2 simultaneous buffer capture starts only.
On 4/6/20 1:54 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 23:38, Sowjanya Komatineni пишет: >> On 4/6/20 1:37 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 06.04.2020 23:20, Sowjanya Komatineni пишет: >>>> On 4/6/20 1:02 PM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>>>> ... >>>>>> +static int chan_capture_kthread_start(void *data) >>>>>> +{ >>>>>> + struct tegra_vi_channel *chan = data; >>>>>> + struct tegra_channel_buffer *buf; >>>>>> + int err = 0; >>>>>> + int caps_inflight; >>>>>> + >>>>>> + set_freezable(); >>>>>> + >>>>>> + while (1) { >>>>>> + try_to_freeze(); >>>>>> + >>>>>> + wait_event_interruptible(chan->start_wait, >>>>>> + !list_empty(&chan->capture) || >>>>>> + kthread_should_stop()); >>>>> Is it really okay that list_empty() isn't protected with a lock? >>>>> >>>>> Why wait_event is "interruptible"? >>>> To allow it to sleep until wakeup on thread it to avoid constant >>>> checking for condition even when no buffers are ready, basically to >>>> prevent blocking. >>> So the "interrupt" is for getting event about kthread_should_stop(), >>> correct? >> also to prevent blocking and to let is sleep and wakeup based on wait >> queue to evaluate condition to proceed with the task > This looks suspicious, the comment to wait_event_interruptible() says > that it will return ERESTARTSYS if signal is recieved.. > > Does this mean that I can send signal from userspace to wake it up? > > The "interruptible" part looks wrong to me. We are not checking for wait_event_interruptible to handle case when it returns ERESTARTSYS. So, signals sent from user space are ignore and we check if when wakeup happens if kthread_stop has requested to stop thread.
On 4/6/20 2:15 PM, Sowjanya Komatineni wrote: > > On 4/6/20 2:11 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>>> Am I understanding correctly that this thread will take 100% CPU, >>>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>>> on more than 2 frames captures, it breaks thread and on next >>>>>>> wakeup it >>>>>>> continues >>>>>> The wait_event() won't wait if condition is true. >>>>> condition is checked when waitqueue is woken up >>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >>>> >>> process is put to sleep until the condition evaluates to true or signal >>> is received. >>> >>> condition is checked each time the waitqueue head is woken up. >> This is a wrong assumption in accordance to the code. > > when every buffer is available as long as we are in streaming, we > should process it. > > So if wake up happens when list has buffer, it will be processed but > at a time we limit processing 2 simultaneous buffer capture starts only. > Fixing typo. I meant when ever buffer is available as long as we are in streaming, we should process it. So capture thread processes as long as buffers are available from user space limiting 2 simultaneous trigger of captures and thread will be in sleep when capture buffers list is empty or no stop thread is signaled.
On 4/6/20 2:39 PM, Sowjanya Komatineni wrote: > > On 4/6/20 2:15 PM, Sowjanya Komatineni wrote: >> >> On 4/6/20 2:11 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>>>> Am I understanding correctly that this thread will take 100% CPU, >>>>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>>>> on more than 2 frames captures, it breaks thread and on next >>>>>>>> wakeup it >>>>>>>> continues >>>>>>> The wait_event() won't wait if condition is true. >>>>>> condition is checked when waitqueue is woken up >>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >>>>> >>>> process is put to sleep until the condition evaluates to true or >>>> signal >>>> is received. >>>> >>>> condition is checked each time the waitqueue head is woken up. >>> This is a wrong assumption in accordance to the code. process is in sleep until the condition is evaluated and when condition is true wakeup still happens only when wake_up on waitqueue is called This is the reason for using this to prevent blocking while waiting for the buffers. >> >> when every buffer is available as long as we are in streaming, we >> should process it. >> >> So if wake up happens when list has buffer, it will be processed but >> at a time we limit processing 2 simultaneous buffer capture starts only. >> > Fixing typo. > > I meant when ever buffer is available as long as we are in streaming, > we should process it. > > So capture thread processes as long as buffers are available from user > space limiting 2 simultaneous trigger of captures and thread will be > in sleep when capture buffers list is empty or no stop thread is > signaled.
On 4/6/20 3:00 PM, Sowjanya Komatineni wrote: > > On 4/6/20 2:39 PM, Sowjanya Komatineni wrote: >> >> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote: >>> >>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>>>>> Am I understanding correctly that this thread will take 100% >>>>>>>>>> CPU, >>>>>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>>>>> on more than 2 frames captures, it breaks thread and on next >>>>>>>>> wakeup it >>>>>>>>> continues >>>>>>>> The wait_event() won't wait if condition is true. >>>>>>> condition is checked when waitqueue is woken up >>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >>>>>> >>>>> process is put to sleep until the condition evaluates to true or >>>>> signal >>>>> is received. >>>>> >>>>> condition is checked each time the waitqueue head is woken up. >>>> This is a wrong assumption in accordance to the code. > > process is in sleep until the condition is evaluated and when > condition is true wakeup still happens only when wake_up on waitqueue > is called > > This is the reason for using this to prevent blocking while waiting > for the buffers. w.r.t capture list update, wakeup happens when wake_up on waitqueue is called. wakeup also happens on kthread stop signal event. > > >>> >>> when every buffer is available as long as we are in streaming, we >>> should process it. >>> >>> So if wake up happens when list has buffer, it will be processed but >>> at a time we limit processing 2 simultaneous buffer capture starts >>> only. >>> >> Fixing typo. >> >> I meant when ever buffer is available as long as we are in streaming, >> we should process it. >> >> So capture thread processes as long as buffers are available from >> user space limiting 2 simultaneous trigger of captures and thread >> will be in sleep when capture buffers list is empty or no stop thread >> is signaled. > > >
07.04.2020 01:07, Sowjanya Komatineni пишет: > > On 4/6/20 3:00 PM, Sowjanya Komatineni wrote: >> >> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote: >>> >>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote: >>>> >>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>>>>>> Am I understanding correctly that this thread will take 100% >>>>>>>>>>> CPU, >>>>>>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>>>>>> on more than 2 frames captures, it breaks thread and on next >>>>>>>>>> wakeup it >>>>>>>>>> continues >>>>>>>>> The wait_event() won't wait if condition is true. >>>>>>>> condition is checked when waitqueue is woken up >>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >>>>>>> >>>>>> process is put to sleep until the condition evaluates to true or >>>>>> signal >>>>>> is received. >>>>>> >>>>>> condition is checked each time the waitqueue head is woken up. >>>>> This is a wrong assumption in accordance to the code. >> >> process is in sleep until the condition is evaluated and when >> condition is true wakeup still happens only when wake_up on waitqueue >> is called >> >> This is the reason for using this to prevent blocking while waiting >> for the buffers. > > w.r.t capture list update, wakeup happens when wake_up on waitqueue is > called. > > wakeup also happens on kthread stop signal event. > >> >> >>>> >>>> when every buffer is available as long as we are in streaming, we >>>> should process it. >>>> >>>> So if wake up happens when list has buffer, it will be processed but >>>> at a time we limit processing 2 simultaneous buffer capture starts >>>> only. >>>> >>> Fixing typo. >>> >>> I meant when ever buffer is available as long as we are in streaming, >>> we should process it. >>> >>> So capture thread processes as long as buffers are available from >>> user space limiting 2 simultaneous trigger of captures and thread >>> will be in sleep when capture buffers list is empty or no stop thread >>> is signaled. IIUC, the waiting won't happen if more than 2 captures are queued and thread will be spinning until captures are processed. I think you need a semaphore with resource count = 2.
On 4/6/20 4:18 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 07.04.2020 01:07, Sowjanya Komatineni пишет: >> On 4/6/20 3:00 PM, Sowjanya Komatineni wrote: >>> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote: >>>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote: >>>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote: >>>>>> External email: Use caution opening links or attachments >>>>>> >>>>>> >>>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>>>>>>> Am I understanding correctly that this thread will take 100% >>>>>>>>>>>> CPU, >>>>>>>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>>>>>>> on more than 2 frames captures, it breaks thread and on next >>>>>>>>>>> wakeup it >>>>>>>>>>> continues >>>>>>>>>> The wait_event() won't wait if condition is true. >>>>>>>>> condition is checked when waitqueue is woken up >>>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >>>>>>>> >>>>>>> process is put to sleep until the condition evaluates to true or >>>>>>> signal >>>>>>> is received. >>>>>>> >>>>>>> condition is checked each time the waitqueue head is woken up. >>>>>> This is a wrong assumption in accordance to the code. >>> process is in sleep until the condition is evaluated and when >>> condition is true wakeup still happens only when wake_up on waitqueue >>> is called >>> >>> This is the reason for using this to prevent blocking while waiting >>> for the buffers. >> w.r.t capture list update, wakeup happens when wake_up on waitqueue is >> called. >> >> wakeup also happens on kthread stop signal event. >> >>> >>>>> when every buffer is available as long as we are in streaming, we >>>>> should process it. >>>>> >>>>> So if wake up happens when list has buffer, it will be processed but >>>>> at a time we limit processing 2 simultaneous buffer capture starts >>>>> only. >>>>> >>>> Fixing typo. >>>> >>>> I meant when ever buffer is available as long as we are in streaming, >>>> we should process it. >>>> >>>> So capture thread processes as long as buffers are available from >>>> user space limiting 2 simultaneous trigger of captures and thread >>>> will be in sleep when capture buffers list is empty or no stop thread >>>> is signaled. > IIUC, the waiting won't happen if more than 2 captures are queued and > thread will be spinning until captures are processed. > > I think you need a semaphore with resource count = 2. we hold on to issuing capture if more than 2 buffers are queued and it continues only after fifo has min 1 slot empty
On 4/6/20 4:48 PM, Sowjanya Komatineni wrote: > > On 4/6/20 4:18 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 07.04.2020 01:07, Sowjanya Komatineni пишет: >>> On 4/6/20 3:00 PM, Sowjanya Komatineni wrote: >>>> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote: >>>>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote: >>>>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote: >>>>>>> External email: Use caution opening links or attachments >>>>>>> >>>>>>> >>>>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>>>>>>>> Am I understanding correctly that this thread will take 100% >>>>>>>>>>>>> CPU, >>>>>>>>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>>>>>>>> on more than 2 frames captures, it breaks thread and on next >>>>>>>>>>>> wakeup it >>>>>>>>>>>> continues >>>>>>>>>>> The wait_event() won't wait if condition is true. >>>>>>>>>> condition is checked when waitqueue is woken up >>>>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >>>>>>>>> >>>>>>>>> >>>>>>>> process is put to sleep until the condition evaluates to true or >>>>>>>> signal >>>>>>>> is received. >>>>>>>> >>>>>>>> condition is checked each time the waitqueue head is woken up. >>>>>>> This is a wrong assumption in accordance to the code. >>>> process is in sleep until the condition is evaluated and when >>>> condition is true wakeup still happens only when wake_up on waitqueue >>>> is called >>>> >>>> This is the reason for using this to prevent blocking while waiting >>>> for the buffers. >>> w.r.t capture list update, wakeup happens when wake_up on waitqueue is >>> called. >>> >>> wakeup also happens on kthread stop signal event. >>> >>>> >>>>>> when every buffer is available as long as we are in streaming, we >>>>>> should process it. >>>>>> >>>>>> So if wake up happens when list has buffer, it will be processed but >>>>>> at a time we limit processing 2 simultaneous buffer capture starts >>>>>> only. >>>>>> >>>>> Fixing typo. >>>>> >>>>> I meant when ever buffer is available as long as we are in streaming, >>>>> we should process it. >>>>> >>>>> So capture thread processes as long as buffers are available from >>>>> user space limiting 2 simultaneous trigger of captures and thread >>>>> will be in sleep when capture buffers list is empty or no stop thread >>>>> is signaled. >> IIUC, the waiting won't happen if more than 2 captures are queued and >> thread will be spinning until captures are processed. >> >> I think you need a semaphore with resource count = 2. > we hold on to issuing capture if more than 2 buffers are queued and it > continues only after fifo has min 1 slot empty caps_inflight gets updated based on requested frame and finished frames and capture will happen only for 2 frames at a time but not more
On 4/6/20 9:11 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2020 18:41, Sowjanya Komatineni пишет: >> On 4/5/20 2:11 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 04.04.2020 04:25, Sowjanya Komatineni пишет: >>> ... >>>> +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi) >>>> +{ >>>> + struct tegra_vi_channel *chan, *tmp; >>>> + unsigned int port_num; >>>> + unsigned int nchannels = vi->soc->vi_max_channels; >>>> + int ret = 0; >>>> + >>>> + for (port_num = 0; port_num < nchannels; port_num++) { >>>> + /* >>>> + * Do not use devm_kzalloc as memory is freed immediately >>>> + * when device instance is unbound but application >>>> might still >>>> + * be holding the device node open. Channel memory >>>> allocated >>>> + * with kzalloc is freed during video device release >>>> callback. >>>> + */ >>>> + chan = kzalloc(sizeof(*chan), GFP_KERNEL); >>> Why anyone would want to unbind this driver in practice? >>> >>> I think it should make more sense to set suppress_bind_attrs=true. >> From the previous feedback of patch series, we need to support >> unbind/bind and looks like this driver should also support to built as a >> module. > If module unloading is also affected, then perhaps you should use > get/put_device() to not allow freeing the resources until they're still > in-use. > > I suppose that it should be up to the V4L core to keep the device alive > while needed, rather than to put the burden to the individual drivers. Hans/Thierry, Can you please comment on this?
04.04.2020 04:25, Sowjanya Komatineni пишет: ... > +static const struct dev_pm_ops tegra_vi_pm_ops = { > + SET_RUNTIME_PM_OPS(vi_runtime_suspend, vi_runtime_resume, NULL) > +}; Aren't the suspend/resume ops needed?
On 4/7/20 12:39 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 04.04.2020 04:25, Sowjanya Komatineni пишет: > ... >> +static const struct dev_pm_ops tegra_vi_pm_ops = { >> + SET_RUNTIME_PM_OPS(vi_runtime_suspend, vi_runtime_resume, NULL) >> +}; > Aren't the suspend/resume ops needed? Complete driver suspend/resume will be implemented later after next series of sensor support
On 4/6/20 4:48 PM, Sowjanya Komatineni wrote: > > On 4/6/20 4:18 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 07.04.2020 01:07, Sowjanya Komatineni пишет: >>> On 4/6/20 3:00 PM, Sowjanya Komatineni wrote: >>>> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote: >>>>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote: >>>>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote: >>>>>>> External email: Use caution opening links or attachments >>>>>>> >>>>>>> >>>>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет: >>>>>>>>>>>>> Am I understanding correctly that this thread will take 100% >>>>>>>>>>>>> CPU, >>>>>>>>>>>>> spinning here, if more than 2 frame-captures queued? >>>>>>>>>>>> on more than 2 frames captures, it breaks thread and on next >>>>>>>>>>>> wakeup it >>>>>>>>>>>> continues >>>>>>>>>>> The wait_event() won't wait if condition is true. >>>>>>>>>> condition is checked when waitqueue is woken up >>>>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 >>>>>>>>> >>>>>>>>> >>>>>>>> process is put to sleep until the condition evaluates to true or >>>>>>>> signal >>>>>>>> is received. >>>>>>>> >>>>>>>> condition is checked each time the waitqueue head is woken up. >>>>>>> This is a wrong assumption in accordance to the code. >>>> process is in sleep until the condition is evaluated and when >>>> condition is true wakeup still happens only when wake_up on waitqueue >>>> is called >>>> >>>> This is the reason for using this to prevent blocking while waiting >>>> for the buffers. >>> w.r.t capture list update, wakeup happens when wake_up on waitqueue is >>> called. >>> >>> wakeup also happens on kthread stop signal event. >>> >>>> >>>>>> when every buffer is available as long as we are in streaming, we >>>>>> should process it. >>>>>> >>>>>> So if wake up happens when list has buffer, it will be processed but >>>>>> at a time we limit processing 2 simultaneous buffer capture starts >>>>>> only. >>>>>> >>>>> Fixing typo. >>>>> >>>>> I meant when ever buffer is available as long as we are in streaming, >>>>> we should process it. >>>>> >>>>> So capture thread processes as long as buffers are available from >>>>> user space limiting 2 simultaneous trigger of captures and thread >>>>> will be in sleep when capture buffers list is empty or no stop thread >>>>> is signaled. >> IIUC, the waiting won't happen if more than 2 captures are queued and >> thread will be spinning until captures are processed. >> >> I think you need a semaphore with resource count = 2. > we hold on to issuing capture if more than 2 buffers are queued and it > continues only after fifo has min 1 slot empty Just want to close on this part of feedback. Hope above explanation is clear regarding triggering/issuing at max 2 frame capture to VI HW and also regarding capture threads where they use wait_event_interruptible to prevent blocking waiting for buffers to be available for captures. So no changes related to this part are needed in v7.
08.04.2020 00:08, Sowjanya Komatineni пишет: ... >>> I think you need a semaphore with resource count = 2. >> we hold on to issuing capture if more than 2 buffers are queued and it >> continues only after fifo has min 1 slot empty > > > Just want to close on this part of feedback. Hope above explanation is > clear regarding triggering/issuing at max 2 frame capture to VI HW and > also regarding capture threads where they use wait_event_interruptible > to prevent blocking waiting for buffers to be available for captures. > > So no changes related to this part are needed in v7. From what I see in the code, you "hold on" by making kthread to spin in a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change should be needed to prevent this. The wait_event_interruptible seems should be okay.
08.04.2020 01:08, Dmitry Osipenko пишет: > 08.04.2020 00:08, Sowjanya Komatineni пишет: > ... >>>> I think you need a semaphore with resource count = 2. >>> we hold on to issuing capture if more than 2 buffers are queued and it >>> continues only after fifo has min 1 slot empty >> >> >> Just want to close on this part of feedback. Hope above explanation is >> clear regarding triggering/issuing at max 2 frame capture to VI HW and >> also regarding capture threads where they use wait_event_interruptible >> to prevent blocking waiting for buffers to be available for captures. >> >> So no changes related to this part are needed in v7. > From what I see in the code, you "hold on" by making kthread to spin in > a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change > should be needed to prevent this. Looks like some other media drivers do: schedule_timeout_uninterruptible(1); to avoid CPU hogging when contention is detected.
On 4/7/20 3:08 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 08.04.2020 00:08, Sowjanya Komatineni пишет: > ... >>>> I think you need a semaphore with resource count = 2. >>> we hold on to issuing capture if more than 2 buffers are queued and it >>> continues only after fifo has min 1 slot empty >> >> Just want to close on this part of feedback. Hope above explanation is >> clear regarding triggering/issuing at max 2 frame capture to VI HW and >> also regarding capture threads where they use wait_event_interruptible >> to prevent blocking waiting for buffers to be available for captures. >> >> So no changes related to this part are needed in v7. > From what I see in the code, you "hold on" by making kthread to spin in > a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change > should be needed to prevent this. > > The wait_event_interruptible seems should be okay. We don't want to prevent that as we already have buffers available for capture so as soon as VI HW issuing single shot is done and when min 1 slot is empty we should continue with issuing for another capture. As long as buffers are available, we should continue to capture and should not hold
08.04.2020 01:22, Sowjanya Komatineni пишет: > > On 4/7/20 3:08 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 08.04.2020 00:08, Sowjanya Komatineni пишет: >> ... >>>>> I think you need a semaphore with resource count = 2. >>>> we hold on to issuing capture if more than 2 buffers are queued and it >>>> continues only after fifo has min 1 slot empty >>> >>> Just want to close on this part of feedback. Hope above explanation is >>> clear regarding triggering/issuing at max 2 frame capture to VI HW and >>> also regarding capture threads where they use wait_event_interruptible >>> to prevent blocking waiting for buffers to be available for captures. >>> >>> So no changes related to this part are needed in v7. >> From what I see in the code, you "hold on" by making kthread to spin in >> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change >> should be needed to prevent this. >> >> The wait_event_interruptible seems should be okay. > > We don't want to prevent that as we already have buffers available for > capture so as soon as VI HW issuing single shot is done and when min 1 > slot is empty we should continue with issuing for another capture. > > As long as buffers are available, we should continue to capture and > should not hold > I suppose that taking a shot takes at least few milliseconds, which should be unacceptable to waste.
On 4/7/20 4:36 PM, Sowjanya Komatineni wrote: > > On 4/7/20 4:12 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 08.04.2020 01:22, Sowjanya Komatineni пишет: >>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 08.04.2020 00:08, Sowjanya Komatineni пишет: >>>> ... >>>>>>> I think you need a semaphore with resource count = 2. >>>>>> we hold on to issuing capture if more than 2 buffers are queued >>>>>> and it >>>>>> continues only after fifo has min 1 slot empty >>>>> Just want to close on this part of feedback. Hope above >>>>> explanation is >>>>> clear regarding triggering/issuing at max 2 frame capture to VI HW >>>>> and >>>>> also regarding capture threads where they use >>>>> wait_event_interruptible >>>>> to prevent blocking waiting for buffers to be available for captures. >>>>> >>>>> So no changes related to this part are needed in v7. >>>> From what I see in the code, you "hold on" by making kthread to >>>> spin in >>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change >>>> should be needed to prevent this. >>>> >>>> The wait_event_interruptible seems should be okay. >>> We don't want to prevent that as we already have buffers available for >>> capture so as soon as VI HW issuing single shot is done and when min 1 >>> slot is empty we should continue with issuing for another capture. >>> >>> As long as buffers are available, we should continue to capture and >>> should not hold >>> >> I suppose that taking a shot takes at least few milliseconds, which >> should be unacceptable to waste. > As long as buffers are in queue we have to keep processing each buffer > and between buffers obviously we have to wait for previous frames to > finish and this why we have separate thread for frame finish where we > can have next buffer capture ready and issue while previous frame > memory write happens
On 4/7/20 4:38 PM, Sowjanya Komatineni wrote: > > On 4/7/20 4:36 PM, Sowjanya Komatineni wrote: >> >> On 4/7/20 4:12 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 08.04.2020 01:22, Sowjanya Komatineni пишет: >>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет: >>>>> ... >>>>>>>> I think you need a semaphore with resource count = 2. >>>>>>> we hold on to issuing capture if more than 2 buffers are queued >>>>>>> and it >>>>>>> continues only after fifo has min 1 slot empty >>>>>> Just want to close on this part of feedback. Hope above >>>>>> explanation is >>>>>> clear regarding triggering/issuing at max 2 frame capture to VI >>>>>> HW and >>>>>> also regarding capture threads where they use >>>>>> wait_event_interruptible >>>>>> to prevent blocking waiting for buffers to be available for >>>>>> captures. >>>>>> >>>>>> So no changes related to this part are needed in v7. >>>>> From what I see in the code, you "hold on" by making kthread to >>>>> spin in >>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change >>>>> should be needed to prevent this. >>>>> >>>>> The wait_event_interruptible seems should be okay. >>>> We don't want to prevent that as we already have buffers available for >>>> capture so as soon as VI HW issuing single shot is done and when min 1 >>>> slot is empty we should continue with issuing for another capture. >>>> >>>> As long as buffers are available, we should continue to capture and >>>> should not hold >>>> >>> I suppose that taking a shot takes at least few milliseconds, which >>> should be unacceptable to waste. >> As long as buffers are in queue we have to keep processing each >> buffer and between buffers obviously we have to wait for previous >> frames to finish and this why we have separate thread for frame >> finish where we can have next buffer capture ready and issue while >> previous frame memory write happens Also we specified numbers buffers as 3 to vb2 queue. So this is rare case but to prevent issuing more than 2 at a time as VI HW is only double buffered and syncpt fifo max depth is 2 added this to be safer.
On 4/7/20 4:38 PM, Sowjanya Komatineni wrote: > > On 4/7/20 4:36 PM, Sowjanya Komatineni wrote: >> >> On 4/7/20 4:12 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 08.04.2020 01:22, Sowjanya Komatineni пишет: >>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет: >>>>> ... >>>>>>>> I think you need a semaphore with resource count = 2. >>>>>>> we hold on to issuing capture if more than 2 buffers are queued >>>>>>> and it >>>>>>> continues only after fifo has min 1 slot empty >>>>>> Just want to close on this part of feedback. Hope above >>>>>> explanation is >>>>>> clear regarding triggering/issuing at max 2 frame capture to VI >>>>>> HW and >>>>>> also regarding capture threads where they use >>>>>> wait_event_interruptible >>>>>> to prevent blocking waiting for buffers to be available for >>>>>> captures. >>>>>> >>>>>> So no changes related to this part are needed in v7. >>>>> From what I see in the code, you "hold on" by making kthread to >>>>> spin in >>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change >>>>> should be needed to prevent this. >>>>> >>>>> The wait_event_interruptible seems should be okay. >>>> We don't want to prevent that as we already have buffers available for >>>> capture so as soon as VI HW issuing single shot is done and when min 1 >>>> slot is empty we should continue with issuing for another capture. >>>> >>>> As long as buffers are available, we should continue to capture and >>>> should not hold >>>> >>> I suppose that taking a shot takes at least few milliseconds, which >>> should be unacceptable to waste. >> As long as buffers are in queue we have to keep processing each >> buffer and between buffers obviously we have to wait for previous >> frames to finish and this why we have separate thread for frame >> finish where we can have next buffer capture ready and issue while >> previous frame memory write happens Also we specified numbers buffers as 3 to vb2 queue. So this is rare case but to prevent issuing more than 2 at a time as VI HW is only double buffered and syncpt fifo max depth is 2 added this to be safer.
On 4/7/20 4:38 PM, Sowjanya Komatineni wrote: > > On 4/7/20 4:36 PM, Sowjanya Komatineni wrote: >> >> On 4/7/20 4:12 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 08.04.2020 01:22, Sowjanya Komatineni пишет: >>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет: >>>>> ... >>>>>>>> I think you need a semaphore with resource count = 2. >>>>>>> we hold on to issuing capture if more than 2 buffers are queued >>>>>>> and it >>>>>>> continues only after fifo has min 1 slot empty >>>>>> Just want to close on this part of feedback. Hope above >>>>>> explanation is >>>>>> clear regarding triggering/issuing at max 2 frame capture to VI >>>>>> HW and >>>>>> also regarding capture threads where they use >>>>>> wait_event_interruptible >>>>>> to prevent blocking waiting for buffers to be available for >>>>>> captures. >>>>>> >>>>>> So no changes related to this part are needed in v7. >>>>> From what I see in the code, you "hold on" by making kthread to >>>>> spin in >>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change >>>>> should be needed to prevent this. >>>>> >>>>> The wait_event_interruptible seems should be okay. >>>> We don't want to prevent that as we already have buffers available for >>>> capture so as soon as VI HW issuing single shot is done and when min 1 >>>> slot is empty we should continue with issuing for another capture. >>>> >>>> As long as buffers are available, we should continue to capture and >>>> should not hold >>>> >>> I suppose that taking a shot takes at least few milliseconds, which >>> should be unacceptable to waste. >> As long as buffers are in queue we have to keep processing each >> buffer and between buffers obviously we have to wait for previous >> frames to finish and this why we have separate thread for frame >> finish where we can have next buffer capture ready and issue while >> previous frame memory write happens Also we specified numbers buffers as 3 to vb2 queue. So this is rare case but to prevent issuing more than 2 at a time as VI HW is only double buffered and syncpt fifo max depth is 2 added this to be safer.
On 4/7/20 4:59 PM, Sowjanya Komatineni wrote: > > On 4/7/20 4:38 PM, Sowjanya Komatineni wrote: >> >> On 4/7/20 4:36 PM, Sowjanya Komatineni wrote: >>> >>> On 4/7/20 4:12 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 08.04.2020 01:22, Sowjanya Komatineni пишет: >>>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote: >>>>>> External email: Use caution opening links or attachments >>>>>> >>>>>> >>>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет: >>>>>> ... >>>>>>>>> I think you need a semaphore with resource count = 2. >>>>>>>> we hold on to issuing capture if more than 2 buffers are queued >>>>>>>> and it >>>>>>>> continues only after fifo has min 1 slot empty >>>>>>> Just want to close on this part of feedback. Hope above >>>>>>> explanation is >>>>>>> clear regarding triggering/issuing at max 2 frame capture to VI >>>>>>> HW and >>>>>>> also regarding capture threads where they use >>>>>>> wait_event_interruptible >>>>>>> to prevent blocking waiting for buffers to be available for >>>>>>> captures. >>>>>>> >>>>>>> So no changes related to this part are needed in v7. >>>>>> From what I see in the code, you "hold on" by making kthread to >>>>>> spin in >>>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change >>>>>> should be needed to prevent this. >>>>>> >>>>>> The wait_event_interruptible seems should be okay. >>>>> We don't want to prevent that as we already have buffers available >>>>> for >>>>> capture so as soon as VI HW issuing single shot is done and when >>>>> min 1 >>>>> slot is empty we should continue with issuing for another capture. >>>>> >>>>> As long as buffers are available, we should continue to capture and >>>>> should not hold >>>>> >>>> I suppose that taking a shot takes at least few milliseconds, which >>>> should be unacceptable to waste. >>> As long as buffers are in queue we have to keep processing each >>> buffer and between buffers obviously we have to wait for previous >>> frames to finish and this why we have separate thread for frame >>> finish where we can have next buffer capture ready and issue while >>> previous frame memory write happens > Also we specified numbers buffers as 3 to vb2 queue. So this is rare > case but to prevent issuing more than 2 at a time as VI HW is only > double buffered and syncpt fifo max depth is 2 added this to be safer. To be more clear, when more buffers are enqueued from userspace always capture list will be full and thread will be busy in capture till either error or stop stream request happens.
08.04.2020 03:00, Sowjanya Komatineni пишет: ... >>>>> I suppose that taking a shot takes at least few milliseconds, which >>>>> should be unacceptable to waste. >>>> As long as buffers are in queue we have to keep processing each >>>> buffer and between buffers obviously we have to wait for previous >>>> frames to finish and this why we have separate thread for frame >>>> finish where we can have next buffer capture ready and issue while >>>> previous frame memory write happens >> Also we specified numbers buffers as 3 to vb2 queue. So this is rare >> case but to prevent issuing more than 2 at a time as VI HW is only >> double buffered and syncpt fifo max depth is 2 added this to be safer. > > To be more clear, when more buffers are enqueued from userspace always > capture list will be full and thread will be busy in capture till either > error or stop stream request happens. > If kthreads take more than 1% of CPU time during capture (video) with more than 2 buffers in queue, then it's not good and I think you should do something about it. If kthreads stay at ~0%, then it should be okay as-is.
On 4/8/20 7:21 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 08.04.2020 03:00, Sowjanya Komatineni пишет: > ... >>>>>> I suppose that taking a shot takes at least few milliseconds, which >>>>>> should be unacceptable to waste. >>>>> As long as buffers are in queue we have to keep processing each >>>>> buffer and between buffers obviously we have to wait for previous >>>>> frames to finish and this why we have separate thread for frame >>>>> finish where we can have next buffer capture ready and issue while >>>>> previous frame memory write happens >>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare >>> case but to prevent issuing more than 2 at a time as VI HW is only >>> double buffered and syncpt fifo max depth is 2 added this to be safer. >> To be more clear, when more buffers are enqueued from userspace always >> capture list will be full and thread will be busy in capture till either >> error or stop stream request happens. >> > If kthreads take more than 1% of CPU time during capture (video) with > more than 2 buffers in queue, then it's not good and I think you should > do something about it. If kthreads stay at ~0%, then it should be okay > as-is. VI outstanding requests max can only be 2 as syncpt fifo depth is 2 and waiting to issue next capture when already 2 captures are inflight happens only during beginning of streaming where buffers allocated go thru capture for first time after queuing. same buffers are returned to userspace after capture and same allocated buffers will be queued back for subsequent captures. So this case of holding to issue single shot when already single shot is issue for 2 frames simultaneous happens only during beginning of start stream and also we set num_buffers to allocate for queue as 3 although 2 is good enough where we will not hit this case even during streaming start with 2 buffers
On 4/8/20 10:45 AM, Sowjanya Komatineni wrote: > > On 4/8/20 7:21 AM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 08.04.2020 03:00, Sowjanya Komatineni пишет: >> ... >>>>>>> I suppose that taking a shot takes at least few milliseconds, which >>>>>>> should be unacceptable to waste. >>>>>> As long as buffers are in queue we have to keep processing each >>>>>> buffer and between buffers obviously we have to wait for previous >>>>>> frames to finish and this why we have separate thread for frame >>>>>> finish where we can have next buffer capture ready and issue while >>>>>> previous frame memory write happens >>>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare >>>> case but to prevent issuing more than 2 at a time as VI HW is only >>>> double buffered and syncpt fifo max depth is 2 added this to be safer. >>> To be more clear, when more buffers are enqueued from userspace always >>> capture list will be full and thread will be busy in capture till >>> either >>> error or stop stream request happens. >>> >> If kthreads take more than 1% of CPU time during capture (video) with >> more than 2 buffers in queue, then it's not good and I think you should >> do something about it. If kthreads stay at ~0%, then it should be okay >> as-is. > > VI outstanding requests max can only be 2 as syncpt fifo depth is 2 > and waiting to issue next capture when already 2 captures are inflight > happens only during beginning of streaming where buffers allocated go > thru capture for first time after queuing. > > same buffers are returned to userspace after capture and same > allocated buffers will be queued back for subsequent captures. > > So this case of holding to issue single shot when already single shot > is issue for 2 frames simultaneous happens only during beginning of > start stream and also we set num_buffers to allocate for queue as 3 > although 2 is good enough where we will not hit this case even during > streaming start with 2 buffers > As 2 buffers are good enough to be clear will update in v7 to use 2 buffers so we don't need to check for more than 2 outstanding buffers.
On 4/8/20 11:58 AM, Sowjanya Komatineni wrote: > > On 4/8/20 10:45 AM, Sowjanya Komatineni wrote: >> >> On 4/8/20 7:21 AM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 08.04.2020 03:00, Sowjanya Komatineni пишет: >>> ... >>>>>>>> I suppose that taking a shot takes at least few milliseconds, >>>>>>>> which >>>>>>>> should be unacceptable to waste. >>>>>>> As long as buffers are in queue we have to keep processing each >>>>>>> buffer and between buffers obviously we have to wait for previous >>>>>>> frames to finish and this why we have separate thread for frame >>>>>>> finish where we can have next buffer capture ready and issue while >>>>>>> previous frame memory write happens >>>>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare >>>>> case but to prevent issuing more than 2 at a time as VI HW is only >>>>> double buffered and syncpt fifo max depth is 2 added this to be >>>>> safer. >>>> To be more clear, when more buffers are enqueued from userspace always >>>> capture list will be full and thread will be busy in capture till >>>> either >>>> error or stop stream request happens. >>>> >>> If kthreads take more than 1% of CPU time during capture (video) with >>> more than 2 buffers in queue, then it's not good and I think you should >>> do something about it. If kthreads stay at ~0%, then it should be okay >>> as-is. >> >> VI outstanding requests max can only be 2 as syncpt fifo depth is 2 >> and waiting to issue next capture when already 2 captures are >> inflight happens only during beginning of streaming where buffers >> allocated go thru capture for first time after queuing. >> >> same buffers are returned to userspace after capture and same >> allocated buffers will be queued back for subsequent captures. >> >> So this case of holding to issue single shot when already single shot >> is issue for 2 frames simultaneous happens only during beginning of >> start stream and also we set num_buffers to allocate for queue as 3 >> although 2 is good enough where we will not hit this case even during >> streaming start with 2 buffers >> > As 2 buffers are good enough to be clear will update in v7 to use 2 > buffers so we don't need to check for more than 2 outstanding buffers. correction: With 3 buffers, as soon as buffer is available capture starts. So right most times I see it waiting for few ms before 3rd capture to get through. As only 2 frames single shot can be issued in sequence (inflight requests), instead of waiting for 1 of the request to finish, we can use 2 buffers and avoid waiting as 2 buffers are good enough. Will change this in v7.
On 4/8/20 12:38 PM, Sowjanya Komatineni wrote: > > On 4/8/20 11:58 AM, Sowjanya Komatineni wrote: >> >> On 4/8/20 10:45 AM, Sowjanya Komatineni wrote: >>> >>> On 4/8/20 7:21 AM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 08.04.2020 03:00, Sowjanya Komatineni пишет: >>>> ... >>>>>>>>> I suppose that taking a shot takes at least few milliseconds, >>>>>>>>> which >>>>>>>>> should be unacceptable to waste. >>>>>>>> As long as buffers are in queue we have to keep processing each >>>>>>>> buffer and between buffers obviously we have to wait for previous >>>>>>>> frames to finish and this why we have separate thread for frame >>>>>>>> finish where we can have next buffer capture ready and issue while >>>>>>>> previous frame memory write happens >>>>>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare >>>>>> case but to prevent issuing more than 2 at a time as VI HW is only >>>>>> double buffered and syncpt fifo max depth is 2 added this to be >>>>>> safer. >>>>> To be more clear, when more buffers are enqueued from userspace >>>>> always >>>>> capture list will be full and thread will be busy in capture till >>>>> either >>>>> error or stop stream request happens. >>>>> >>>> If kthreads take more than 1% of CPU time during capture (video) with >>>> more than 2 buffers in queue, then it's not good and I think you >>>> should >>>> do something about it. If kthreads stay at ~0%, then it should be okay >>>> as-is. >>> >>> VI outstanding requests max can only be 2 as syncpt fifo depth is >>> 2 and waiting to issue next capture when already 2 captures are >>> inflight happens only during beginning of streaming where buffers >>> allocated go thru capture for first time after queuing. >>> >>> same buffers are returned to userspace after capture and same >>> allocated buffers will be queued back for subsequent captures. >>> >>> So this case of holding to issue single shot when already single >>> shot is issue for 2 frames simultaneous happens only during >>> beginning of start stream and also we set num_buffers to allocate >>> for queue as 3 although 2 is good enough where we will not hit this >>> case even during streaming start with 2 buffers >>> >> As 2 buffers are good enough to be clear will update in v7 to use 2 >> buffers so we don't need to check for more than 2 outstanding buffers. > > correction: With 3 buffers, as soon as buffer is available capture > starts. So right most times I see it waiting for few ms before 3rd > capture to get through. > > As only 2 frames single shot can be issued in sequence (inflight > requests), instead of waiting for 1 of the request to finish, we can > use 2 buffers and avoid waiting as 2 buffers are good enough. Will > change this in v7. > > > Tested with 3 buffers and by checking outstanding buffers in process by VI hw and holding to start capture till one outstanding buffer in process by HW. Also tested with 2 buffers without checking for outstanding buffers. In both cases, I see same %CPU for the kthreads and is < 1%
09.04.2020 06:38, Sowjanya Komatineni пишет: ... > Tested with 3 buffers and by checking outstanding buffers in process by > VI hw and holding to start capture till one outstanding buffer in > process by HW. > Also tested with 2 buffers without checking for outstanding buffers. > > In both cases, I see same %CPU for the kthreads and is < 1% > I don't see where buffers queue max limit is set to 3 in the code, but should be okay if CPU isn't getting hogged. Looking forward to v7.
On 4/9/20 7:50 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 09.04.2020 06:38, Sowjanya Komatineni пишет: > ... >> Tested with 3 buffers and by checking outstanding buffers in process by >> VI hw and holding to start capture till one outstanding buffer in >> process by HW. >> Also tested with 2 buffers without checking for outstanding buffers. >> >> In both cases, I see same %CPU for the kthreads and is < 1% >> > I don't see where buffers queue max limit is set to 3 in the code, but > should be okay if CPU isn't getting hogged. Looking forward to v7. Sorry, correction I meant to say pre-queued buffers before streaming not num_buffers. vb2 queue min_buffers_needed was set to 3 as part of one of the issue debug in earlier version which actually was irrelevant to that issue and should have been removed. Will remove min_buffers_needed in v7. I added checking for outstanding requests by hardware just to be safer although we may not hit this case of issuing more than 1 outstanding frame capture to VI hardware as capture_frame() waits till it sees frame start event through HW syncpt increment before proceeding for memory write and issuing next frame capture. So issuing frame captures are synchronized with frame start and frame end. Will remove min_buffers_needed and also explicit check for outstanding buffers in v7.
09.04.2020 21:28, Sowjanya Komatineni пишет: > > On 4/9/20 7:50 AM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 09.04.2020 06:38, Sowjanya Komatineni пишет: >> ... >>> Tested with 3 buffers and by checking outstanding buffers in process by >>> VI hw and holding to start capture till one outstanding buffer in >>> process by HW. >>> Also tested with 2 buffers without checking for outstanding buffers. >>> >>> In both cases, I see same %CPU for the kthreads and is < 1% >>> >> I don't see where buffers queue max limit is set to 3 in the code, but >> should be okay if CPU isn't getting hogged. Looking forward to v7. > Sorry, correction I meant to say pre-queued buffers before streaming not > num_buffers. > vb2 queue min_buffers_needed was set to 3 as part of one of the issue > debug in earlier version which actually was irrelevant to that issue and > should have been removed. Will remove min_buffers_needed in v7. > > I added checking for outstanding requests by hardware just to be safer > although we may not hit this case of issuing more than 1 outstanding > frame capture to VI hardware as capture_frame() waits till it sees frame > start event through HW syncpt increment before proceeding for memory > write and issuing next frame capture. > > So issuing frame captures are synchronized with frame start and frame end. > > Will remove min_buffers_needed and also explicit check for outstanding > buffers in v7. It's still not clear to me how the "pre-queued buffers" will be limited. I'll take another look at the v7.
On 4/10/20 11:47 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 09.04.2020 21:28, Sowjanya Komatineni пишет: >> On 4/9/20 7:50 AM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 09.04.2020 06:38, Sowjanya Komatineni пишет: >>> ... >>>> Tested with 3 buffers and by checking outstanding buffers in process by >>>> VI hw and holding to start capture till one outstanding buffer in >>>> process by HW. >>>> Also tested with 2 buffers without checking for outstanding buffers. >>>> >>>> In both cases, I see same %CPU for the kthreads and is < 1% >>>> >>> I don't see where buffers queue max limit is set to 3 in the code, but >>> should be okay if CPU isn't getting hogged. Looking forward to v7. >> Sorry, correction I meant to say pre-queued buffers before streaming not >> num_buffers. >> vb2 queue min_buffers_needed was set to 3 as part of one of the issue >> debug in earlier version which actually was irrelevant to that issue and >> should have been removed. Will remove min_buffers_needed in v7. >> >> I added checking for outstanding requests by hardware just to be safer >> although we may not hit this case of issuing more than 1 outstanding >> frame capture to VI hardware as capture_frame() waits till it sees frame >> start event through HW syncpt increment before proceeding for memory >> write and issuing next frame capture. >> >> So issuing frame captures are synchronized with frame start and frame end. >> >> Will remove min_buffers_needed and also explicit check for outstanding >> buffers in v7. > It's still not clear to me how the "pre-queued buffers" will be limited. > I'll take another look at the v7. OK, but I don't understand what you mean by limit on pre-queued buffers. I was saying vb2 queue has min_buffers_needed which was set to 3 where streaming will start only after 3 buffers got queued up. Regarding outstanding condition check to make sure no more than 2 syncpt trigger requests are in FIFO I added it to be safe where mostly we may not hit and also I only see capture start thread holding for it during initial frame capture as it issues single shot for 1st 2 buffers capture and holds 3 buffers which is already queued till at least one of those 2 issued capture is done to make sure of not triggering syncpt condition when fifo already has 2 pending. In v7, will remove setting min_buffers_needed and also outstanding syncpt trigger check.
10.04.2020 21:59, Sowjanya Komatineni пишет: ... >> It's still not clear to me how the "pre-queued buffers" will be limited. >> I'll take another look at the v7. > > OK, but I don't understand what you mean by limit on pre-queued buffers. > > I was saying vb2 queue has min_buffers_needed which was set to 3 where > streaming will start only after 3 buffers got queued up. > > Regarding outstanding condition check to make sure no more than 2 syncpt > trigger requests are in FIFO I added it to be safe where mostly we may > not hit and also I only see capture start thread holding for it during > initial frame capture as it issues single shot for 1st 2 buffers capture > and holds 3 buffers which is already queued till at least one of those 2 > issued capture is done to make sure of not triggering syncpt condition > when fifo already has 2 pending. > > In v7, will remove setting min_buffers_needed and also outstanding > syncpt trigger check. Okay, seems I got what you're saying. Yes, the check should be removed. It's impossible to get the frame-start event while capture of the previous buffer is in-progress.
04.04.2020 04:25, Sowjanya Komatineni пишет: > + /* wait for syncpt counter to reach frame start event threshold */ > + err = host1x_syncpt_wait(chan->frame_start_sp, thresh, > + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value); > + if (err) { > + dev_err(&chan->video.dev, > + "frame start syncpt timeout: %d\n", err); I guess this and the other timeout should be dev_err_ratelimited().
This series adds Tegra210 VI and CSI driver for built-in test pattern generator (TPG) capture. Tegra210 supports max 6 channels on VI and 6 ports on CSI where each CSI port is one-to-one mapped to VI channel for video capture. This series has TPG support only where it creates hard media links between CSI subdevice and VI video device without device graphs. v4l2-compliance results are available below the patch diff. [v6]: Includes, - v5 feedback - fix for csi_tpg clock parent - fix to free channel resources in video device release callback for registered video devices as resource might still be in use when application holds handle to it during driver unbind. - added blanking intervals based on resolution and bpp for csi internal tpg. - added implementation for subdev pad ops enum_frame_size and enum_frame_interval. [v5]: Includes, - v4 feedback - fix for venc powergate mc reset order. - fix to have unbind and bind work during v4l2-ctl sleep and streaming. [v4]: Includes, - v3 feedback changes and some improvements - Fixes tegra_channel_buffer struct to use v4l2 buffer as first member. This also fixes crash of unable to handle kernel write to read-only memory. - Uses separate host1x sync ids for frame start and memory write ack as single sync id for both can cause sync loss between exact frame start and memory write ack events. - Uses client managed host1x syncpoints. - Includes fix to increment syncpoint counter to match cached value to synchronize in case of timeouts or missed hardware triggers. - Frame start and memory write ack syncpoint FIFO's are of size 2. So, updated capture logic to avoid adding more than 2 sync point condition requests to FIFOs to avoid overflow. - Implemented PM ops for runtime suspend and resume along with generic power domains to allow proper power gate and ungate sequencing along with MC VI flush during power gate. - Fixed Tegra210 device tree sor power domain clocks. - Added missing reset-cells to mc node. [v3]: Includes, - video device node handling set/get formats of all devices in the pipeline. - Removed subdev nodes. - Fixed frame sync timeout issue due to CSI clocks not properly set for corresponding blocks. - uses minimum 3 buffers to be queued to fixed memory race between DMA writes and userspace reads causing kernel hang reporting kernel write to read-only memory. - Improved capture threads and done threads to avoid possible race conditions and added recovery in case of frame sync timeout. - Passes all the V4L compliance tests. [v2]: Includes, - v0 feedback - Merged files to have Tegra specific separately - Moved CSI device as child to VI as Tegra210 CSI is part of VI sharing same host interface and register space. - Added link_validate for format validation. - Fixes for passing v4l2-compliance for media, video, and subdevices. [v1]: Includes, - Adds CSI TPG clock to Tegra210 clock driver - Host1x video driver with VI and CSI clients. - Support for Tegra210 only. - VI CSI TPG support with hard media links in driver. - Video formats supported by Tegra210 VI - CSI TPG supported video formats Sowjanya Komatineni (9): arm64: tegra: Fix sor powergate clocks and reset arm64: tegra: Add reset-cells to mc dt-bindings: clock: tegra: Add clk id for CSI TPG clock clk: tegra: Add Tegra210 CSI TPG clock gate dt-binding: tegra: Add VI and CSI bindings media: tegra: Add Tegra210 Video input driver MAINTAINERS: Add Tegra Video driver section dt-bindings: reset: Add ID for Tegra210 VI reset arm64: tegra: Add Tegra VI CSI support in device tree .../display/tegra/nvidia,tegra20-host1x.txt | 73 +- MAINTAINERS | 10 + arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 10 + arch/arm64/boot/dts/nvidia/tegra210.dtsi | 52 +- drivers/clk/tegra/clk-tegra210.c | 7 + 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 | 13 + drivers/staging/media/tegra/tegra-common.h | 263 +++++ drivers/staging/media/tegra/tegra-csi.c | 639 +++++++++++ drivers/staging/media/tegra/tegra-csi.h | 147 +++ drivers/staging/media/tegra/tegra-vi.c | 1181 ++++++++++++++++++++ drivers/staging/media/tegra/tegra-vi.h | 83 ++ drivers/staging/media/tegra/tegra-video.c | 131 +++ drivers/staging/media/tegra/tegra-video.h | 32 + drivers/staging/media/tegra/tegra210.c | 718 ++++++++++++ drivers/staging/media/tegra/tegra210.h | 192 ++++ include/dt-bindings/clock/tegra210-car.h | 2 +- include/dt-bindings/reset/tegra210-car.h | 1 + 21 files changed, 3558 insertions(+), 17 deletions(-) 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 v4l2-compliance SHA: 81e45d957c4db39397f893100b3d2729ef39b052, 32 bits, 32-bit time_t Compliance test for tegra-video device /dev/media0: Media Driver Info: Driver name : tegra-video Model : NVIDIA Tegra Video Input Device Serial : Bus info : platform:54080000.vi Media version : 5.6.0 Hardware revision: 0x00000003 (3) Driver version : 5.6.0 Required ioctls: test MEDIA_IOC_DEVICE_INFO: OK Allow for multiple opens: test second /dev/media0 open: OK test MEDIA_IOC_DEVICE_INFO: OK test for unlimited opens: OK Media Controller ioctls: test MEDIA_IOC_G_TOPOLOGY: OK Entities: 12 Interfaces: 6 Pads: 12 Links: 12 test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK test MEDIA_IOC_SETUP_LINK: OK test invalid ioctls: OK Total for tegra-video device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for tegra-video device /dev/video0: Driver Info: Driver name : tegra-video Card type : 54080000.vi-output-0 Bus info : platform:54080000.vi Driver version : 5.6.0 Capabilities : 0x85200001 Video Capture Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05200001 Video Capture Read/Write Streaming Extended Pix Format Media Driver Info: Driver name : tegra-video Model : NVIDIA Tegra Video Input Device Serial : Bus info : platform:54080000.vi Media version : 5.6.0 Hardware revision: 0x00000003 (3) Driver version : 5.6.0 Interface Info: ID : 0x03000003 Type : V4L Video Entity Info: ID : 0x00000001 (1) Name : 54080000.vi-output-0 Function : V4L2 I/O Pad 0x01000002 : 0: Sink Link 0x02000007: from remote pad 0x1000006 of entity 'tpg-0': Data, Enabled Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video0 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK test blocking wait: OK test MMAP (no poll): OK test MMAP (select): OK test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for tegra-video device /dev/video0: 53, Succeeded: 53, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for tegra-video device /dev/video1: Driver Info: Driver name : tegra-video Card type : 54080000.vi-output-1 Bus info : platform:54080000.vi Driver version : 5.6.0 Capabilities : 0x85200001 Video Capture Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05200001 Video Capture Read/Write Streaming Extended Pix Format Media Driver Info: Driver name : tegra-video Model : NVIDIA Tegra Video Input Device Serial : Bus info : platform:54080000.vi Media version : 5.6.0 Hardware revision: 0x00000003 (3) Driver version : 5.6.0 Interface Info: ID : 0x0300000b Type : V4L Video Entity Info: ID : 0x00000009 (9) Name : 54080000.vi-output-1 Function : V4L2 I/O Pad 0x0100000a : 0: Sink Link 0x0200000f: from remote pad 0x100000e of entity 'tpg-1': Data, Enabled Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video1 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK test blocking wait: OK test MMAP (no poll): OK test MMAP (select): OK test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for tegra-video device /dev/video1: 53, Succeeded: 53, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for tegra-video device /dev/video2: Driver Info: Driver name : tegra-video Card type : 54080000.vi-output-2 Bus info : platform:54080000.vi Driver version : 5.6.0 Capabilities : 0x85200001 Video Capture Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05200001 Video Capture Read/Write Streaming Extended Pix Format Media Driver Info: Driver name : tegra-video Model : NVIDIA Tegra Video Input Device Serial : Bus info : platform:54080000.vi Media version : 5.6.0 Hardware revision: 0x00000003 (3) Driver version : 5.6.0 Interface Info: ID : 0x03000013 Type : V4L Video Entity Info: ID : 0x00000011 (17) Name : 54080000.vi-output-2 Function : V4L2 I/O Pad 0x01000012 : 0: Sink Link 0x02000017: from remote pad 0x1000016 of entity 'tpg-2': Data, Enabled Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video2 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK test blocking wait: OK test MMAP (no poll): OK test MMAP (select): OK test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for tegra-video device /dev/video2: 53, Succeeded: 53, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for tegra-video device /dev/video3: Driver Info: Driver name : tegra-video Card type : 54080000.vi-output-3 Bus info : platform:54080000.vi Driver version : 5.6.0 Capabilities : 0x85200001 Video Capture Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05200001 Video Capture Read/Write Streaming Extended Pix Format Media Driver Info: Driver name : tegra-video Model : NVIDIA Tegra Video Input Device Serial : Bus info : platform:54080000.vi Media version : 5.6.0 Hardware revision: 0x00000003 (3) Driver version : 5.6.0 Interface Info: ID : 0x0300001b Type : V4L Video Entity Info: ID : 0x00000019 (25) Name : 54080000.vi-output-3 Function : V4L2 I/O Pad 0x0100001a : 0: Sink Link 0x0200001f: from remote pad 0x100001e of entity 'tpg-3': Data, Enabled Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video3 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK test blocking wait: OK test MMAP (no poll): OK test MMAP (select): OK test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for tegra-video device /dev/video3: 53, Succeeded: 53, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for tegra-video device /dev/video4: Driver Info: Driver name : tegra-video Card type : 54080000.vi-output-4 Bus info : platform:54080000.vi Driver version : 5.6.0 Capabilities : 0x85200001 Video Capture Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05200001 Video Capture Read/Write Streaming Extended Pix Format Media Driver Info: Driver name : tegra-video Model : NVIDIA Tegra Video Input Device Serial : Bus info : platform:54080000.vi Media version : 5.6.0 Hardware revision: 0x00000003 (3) Driver version : 5.6.0 Interface Info: ID : 0x03000023 Type : V4L Video Entity Info: ID : 0x00000021 (33) Name : 54080000.vi-output-4 Function : V4L2 I/O Pad 0x01000022 : 0: Sink Link 0x02000027: from remote pad 0x1000026 of entity 'tpg-4': Data, Enabled Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video4 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK test blocking wait: OK test MMAP (no poll): OK test MMAP (select): OK test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for tegra-video device /dev/video4: 53, Succeeded: 53, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for tegra-video device /dev/video5: Driver Info: Driver name : tegra-video Card type : 54080000.vi-output-5 Bus info : platform:54080000.vi Driver version : 5.6.0 Capabilities : 0x85200001 Video Capture Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05200001 Video Capture Read/Write Streaming Extended Pix Format Media Driver Info: Driver name : tegra-video Model : NVIDIA Tegra Video Input Device Serial : Bus info : platform:54080000.vi Media version : 5.6.0 Hardware revision: 0x00000003 (3) Driver version : 5.6.0 Interface Info: ID : 0x0300002b Type : V4L Video Entity Info: ID : 0x00000029 (41) Name : 54080000.vi-output-5 Function : V4L2 I/O Pad 0x0100002a : 0: Sink Link 0x0200002f: from remote pad 0x100002e of entity 'tpg-5': Data, Enabled Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video5 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK test blocking wait: OK test MMAP (no poll): OK test MMAP (select): OK test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for tegra-video device /dev/video5: 53, Succeeded: 53, Failed: 0, Warnings: 0 Grand Total for tegra-video device /dev/media0: 326, Succeeded: 326, Failed: 0, Warnings: 0