* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
@ 2020-04-05 19:37 ` Dmitry Osipenko
2020-04-06 18:58 ` Sowjanya Komatineni
2020-04-05 19:45 ` Dmitry Osipenko
` (9 subsequent siblings)
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-05 19:37 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-05 19:37 ` [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver Dmitry Osipenko
@ 2020-04-06 18:58 ` Sowjanya Komatineni
0 siblings, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 18:58 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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...
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
2020-04-05 19:37 ` [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver Dmitry Osipenko
@ 2020-04-05 19:45 ` Dmitry Osipenko
2020-04-05 19:57 ` Dmitry Osipenko
2020-04-05 19:51 ` Dmitry Osipenko
` (8 subsequent siblings)
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-05 19:45 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-05 19:45 ` Dmitry Osipenko
@ 2020-04-05 19:57 ` Dmitry Osipenko
0 siblings, 0 replies; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-05 19:57 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
2020-04-05 19:37 ` [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver Dmitry Osipenko
2020-04-05 19:45 ` Dmitry Osipenko
@ 2020-04-05 19:51 ` Dmitry Osipenko
2020-04-05 20:35 ` Dmitry Osipenko
` (7 subsequent siblings)
10 siblings, 0 replies; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-05 19:51 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (2 preceding siblings ...)
2020-04-05 19:51 ` Dmitry Osipenko
@ 2020-04-05 20:35 ` Dmitry Osipenko
2020-04-06 15:35 ` Sowjanya Komatineni
2020-04-05 20:54 ` Dmitry Osipenko
` (6 subsequent siblings)
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-05 20:35 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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;
> + }
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-05 20:35 ` Dmitry Osipenko
@ 2020-04-06 15:35 ` Sowjanya Komatineni
2020-04-06 16:05 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 15:35 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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;
>> + }
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 15:35 ` Sowjanya Komatineni
@ 2020-04-06 16:05 ` Dmitry Osipenko
2020-04-06 16:12 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 16:05 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 16:05 ` Dmitry Osipenko
@ 2020-04-06 16:12 ` Sowjanya Komatineni
2020-04-06 16:29 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 16:12 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 16:12 ` Sowjanya Komatineni
@ 2020-04-06 16:29 ` Dmitry Osipenko
2020-04-06 16:37 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 16:29 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 16:29 ` Dmitry Osipenko
@ 2020-04-06 16:37 ` Sowjanya Komatineni
2020-04-06 17:02 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 16:37 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 16:37 ` Sowjanya Komatineni
@ 2020-04-06 17:02 ` Sowjanya Komatineni
2020-04-06 19:53 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 17:02 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 17:02 ` Sowjanya Komatineni
@ 2020-04-06 19:53 ` Dmitry Osipenko
2020-04-06 20:05 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 19:53 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
>
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 19:53 ` Dmitry Osipenko
@ 2020-04-06 20:05 ` Sowjanya Komatineni
2020-04-06 20:28 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:05 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
>>
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:05 ` Sowjanya Komatineni
@ 2020-04-06 20:28 ` Dmitry Osipenko
2020-04-06 20:30 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 20:28 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:28 ` Dmitry Osipenko
@ 2020-04-06 20:30 ` Sowjanya Komatineni
0 siblings, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:30 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (3 preceding siblings ...)
2020-04-05 20:35 ` Dmitry Osipenko
@ 2020-04-05 20:54 ` Dmitry Osipenko
2020-04-05 21:11 ` Dmitry Osipenko
` (5 subsequent siblings)
10 siblings, 0 replies; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-05 20:54 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (4 preceding siblings ...)
2020-04-05 20:54 ` Dmitry Osipenko
@ 2020-04-05 21:11 ` Dmitry Osipenko
2020-04-06 15:41 ` Sowjanya Komatineni
2020-04-06 19:48 ` Dmitry Osipenko
` (4 subsequent siblings)
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-05 21:11 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-05 21:11 ` Dmitry Osipenko
@ 2020-04-06 15:41 ` Sowjanya Komatineni
2020-04-06 16:11 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 15:41 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 15:41 ` Sowjanya Komatineni
@ 2020-04-06 16:11 ` Dmitry Osipenko
2020-04-07 19:05 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 16:11 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 16:11 ` Dmitry Osipenko
@ 2020-04-07 19:05 ` Sowjanya Komatineni
0 siblings, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-07 19:05 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (5 preceding siblings ...)
2020-04-05 21:11 ` Dmitry Osipenko
@ 2020-04-06 19:48 ` Dmitry Osipenko
2020-04-06 20:00 ` Sowjanya Komatineni
2020-04-06 20:02 ` Dmitry Osipenko
` (3 subsequent siblings)
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 19:48 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 19:48 ` Dmitry Osipenko
@ 2020-04-06 20:00 ` Sowjanya Komatineni
0 siblings, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:00 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (6 preceding siblings ...)
2020-04-06 19:48 ` Dmitry Osipenko
@ 2020-04-06 20:02 ` Dmitry Osipenko
2020-04-06 20:20 ` Sowjanya Komatineni
2020-04-06 20:45 ` Dmitry Osipenko
` (2 subsequent siblings)
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 20:02 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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;
> +}
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:02 ` Dmitry Osipenko
@ 2020-04-06 20:20 ` Sowjanya Komatineni
2020-04-06 20:37 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:20 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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;
>> +}
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:20 ` Sowjanya Komatineni
@ 2020-04-06 20:37 ` Dmitry Osipenko
2020-04-06 20:38 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 20:37 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:37 ` Dmitry Osipenko
@ 2020-04-06 20:38 ` Sowjanya Komatineni
2020-04-06 20:43 ` Sowjanya Komatineni
2020-04-06 20:54 ` Dmitry Osipenko
0 siblings, 2 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:38 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
>
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:38 ` Sowjanya Komatineni
@ 2020-04-06 20:43 ` Sowjanya Komatineni
2020-04-06 20:54 ` Dmitry Osipenko
1 sibling, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:43 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
>>
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:38 ` Sowjanya Komatineni
2020-04-06 20:43 ` Sowjanya Komatineni
@ 2020-04-06 20:54 ` Dmitry Osipenko
2020-04-06 21:18 ` Sowjanya Komatineni
1 sibling, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 20:54 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:54 ` Dmitry Osipenko
@ 2020-04-06 21:18 ` Sowjanya Komatineni
0 siblings, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 21:18 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (7 preceding siblings ...)
2020-04-06 20:02 ` Dmitry Osipenko
@ 2020-04-06 20:45 ` Dmitry Osipenko
2020-04-06 20:50 ` Sowjanya Komatineni
2020-04-07 19:39 ` Dmitry Osipenko
2020-04-10 19:47 ` Dmitry Osipenko
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 20:45 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:45 ` Dmitry Osipenko
@ 2020-04-06 20:50 ` Sowjanya Komatineni
2020-04-06 20:53 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:50 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:50 ` Sowjanya Komatineni
@ 2020-04-06 20:53 ` Dmitry Osipenko
2020-04-06 20:55 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 20:53 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:53 ` Dmitry Osipenko
@ 2020-04-06 20:55 ` Sowjanya Komatineni
2020-04-06 20:56 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 20:55 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:55 ` Sowjanya Komatineni
@ 2020-04-06 20:56 ` Dmitry Osipenko
2020-04-06 21:02 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 20:56 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 20:56 ` Dmitry Osipenko
@ 2020-04-06 21:02 ` Sowjanya Komatineni
2020-04-06 21:11 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 21:02 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 21:02 ` Sowjanya Komatineni
@ 2020-04-06 21:11 ` Dmitry Osipenko
2020-04-06 21:15 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 21:11 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 21:11 ` Dmitry Osipenko
@ 2020-04-06 21:15 ` Sowjanya Komatineni
2020-04-06 21:39 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 21:15 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 21:15 ` Sowjanya Komatineni
@ 2020-04-06 21:39 ` Sowjanya Komatineni
2020-04-06 22:00 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 21:39 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 21:39 ` Sowjanya Komatineni
@ 2020-04-06 22:00 ` Sowjanya Komatineni
2020-04-06 22:07 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 22:00 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 22:00 ` Sowjanya Komatineni
@ 2020-04-06 22:07 ` Sowjanya Komatineni
2020-04-06 23:18 ` Dmitry Osipenko
0 siblings, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 22:07 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
>
>
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 22:07 ` Sowjanya Komatineni
@ 2020-04-06 23:18 ` Dmitry Osipenko
2020-04-06 23:48 ` Sowjanya Komatineni
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-06 23:18 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 23:18 ` Dmitry Osipenko
@ 2020-04-06 23:48 ` Sowjanya Komatineni
2020-04-06 23:50 ` Sowjanya Komatineni
2020-04-07 21:08 ` Sowjanya Komatineni
0 siblings, 2 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 23:48 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 23:48 ` Sowjanya Komatineni
@ 2020-04-06 23:50 ` Sowjanya Komatineni
2020-04-07 21:08 ` Sowjanya Komatineni
1 sibling, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-06 23:50 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-06 23:48 ` Sowjanya Komatineni
2020-04-06 23:50 ` Sowjanya Komatineni
@ 2020-04-07 21:08 ` Sowjanya Komatineni
2020-04-07 22:08 ` Dmitry Osipenko
1 sibling, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-07 21:08 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-07 21:08 ` Sowjanya Komatineni
@ 2020-04-07 22:08 ` Dmitry Osipenko
2020-04-07 22:14 ` Dmitry Osipenko
2020-04-07 22:22 ` Sowjanya Komatineni
0 siblings, 2 replies; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-07 22:08 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-07 22:08 ` Dmitry Osipenko
@ 2020-04-07 22:14 ` Dmitry Osipenko
2020-04-07 22:22 ` Sowjanya Komatineni
1 sibling, 0 replies; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-07 22:14 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-07 22:08 ` Dmitry Osipenko
2020-04-07 22:14 ` Dmitry Osipenko
@ 2020-04-07 22:22 ` Sowjanya Komatineni
2020-04-07 23:12 ` Dmitry Osipenko
1 sibling, 1 reply; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-07 22:22 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-07 22:22 ` Sowjanya Komatineni
@ 2020-04-07 23:12 ` Dmitry Osipenko
[not found] ` <1a31cd60-739f-0660-1c45-31487d2f2128@nvidia.com>
0 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-07 23:12 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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.
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (8 preceding siblings ...)
2020-04-06 20:45 ` Dmitry Osipenko
@ 2020-04-07 19:39 ` Dmitry Osipenko
2020-04-07 19:42 ` Sowjanya Komatineni
2020-04-10 19:47 ` Dmitry Osipenko
10 siblings, 1 reply; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-07 19:39 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 79+ messages in thread* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
2020-04-07 19:39 ` Dmitry Osipenko
@ 2020-04-07 19:42 ` Sowjanya Komatineni
0 siblings, 0 replies; 79+ messages in thread
From: Sowjanya Komatineni @ 2020-04-07 19:42 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver
[not found] ` <1585963507-12610-7-git-send-email-skomatineni@nvidia.com>
` (9 preceding siblings ...)
2020-04-07 19:39 ` Dmitry Osipenko
@ 2020-04-10 19:47 ` Dmitry Osipenko
10 siblings, 0 replies; 79+ messages in thread
From: Dmitry Osipenko @ 2020-04-10 19:47 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
sakari.ailus, helen.koike
Cc: sboyd, linux-media, devicetree, linux-clk, linux-tegra,
linux-kernel
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().
^ permalink raw reply [flat|nested] 79+ messages in thread