2020-04-28 04:24:07

by Sowjanya Komatineni

[permalink] [raw]
Subject: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming

PM runtime resume and suspend should happen during streaming start
and stop so only when there is any active streaming from any of the
video devices, power and clocks gets enabled otherwise they will be
disabled.

This patch moves pm_runtime_get_sync and pm_runtime_put_sync to
streaming start and stop for both Tegra VI and CSI.

Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/staging/media/tegra-video/csi.c | 27 +++++++++++++++++---------
drivers/staging/media/tegra-video/csi.h | 2 ++
drivers/staging/media/tegra-video/vi.c | 34 ++++++++++++++++++++-------------
drivers/staging/media/tegra-video/vi.h | 2 ++
4 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index b3dd0c3..29ccdae 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
struct tegra_csi *csi = csi_chan->csi;
+ int ret;
+
+ if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
+ 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);
+ atomic_dec(&csi->clk_refcnt);
+ return ret;
+ }
+ }
+
+ ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);

- return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
+ if ((ret < 0 || !enable) && atomic_dec_and_test(&csi->clk_refcnt))
+ pm_runtime_put_sync(csi->dev);
+
+ return ret;
}

/*
@@ -437,13 +454,6 @@ static int tegra_csi_init(struct host1x_client *client)

INIT_LIST_HEAD(&csi->csi_chans);

- ret = pm_runtime_get_sync(csi->dev);
- if (ret < 0) {
- dev_err(csi->dev, "failed to get runtime PM: %d\n", ret);
- pm_runtime_put_noidle(csi->dev);
- return ret;
- }
-
ret = tegra_csi_tpg_channels_alloc(csi);
if (ret < 0) {
dev_err(csi->dev,
@@ -470,7 +480,6 @@ static int tegra_csi_exit(struct host1x_client *client)
struct tegra_csi *csi = host1x_client_to_csi(client);

tegra_csi_channels_cleanup(csi);
- pm_runtime_put_sync(csi->dev);

return 0;
}
diff --git a/drivers/staging/media/tegra-video/csi.h b/drivers/staging/media/tegra-video/csi.h
index 217d5d3..4781b83 100644
--- a/drivers/staging/media/tegra-video/csi.h
+++ b/drivers/staging/media/tegra-video/csi.h
@@ -125,6 +125,7 @@ struct tegra_csi_soc {
* @soc: pointer to SoC data structure
* @ops: csi operations
* @channels: list head for CSI channels
+ * @clk_refcnt: reference counter for clock enable/disable
*/
struct tegra_csi {
struct device *dev;
@@ -134,6 +135,7 @@ struct tegra_csi {
const struct tegra_csi_soc *soc;
const struct tegra_csi_ops *ops;
struct list_head csi_chans;
+ atomic_t clk_refcnt;
};

void tegra_csi_error_recover(struct v4l2_subdev *subdev);
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 61ecc2b..cd4c08d 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -200,8 +200,25 @@ void tegra_channel_release_buffers(struct tegra_vi_channel *chan,
static int tegra_channel_start_streaming(struct vb2_queue *vq, u32 count)
{
struct tegra_vi_channel *chan = vb2_get_drv_priv(vq);
+ int ret;

- return chan->vi->ops->vi_start_streaming(vq, count);
+ if (atomic_add_return(1, &chan->vi->power_on_refcnt) == 1) {
+ ret = pm_runtime_get_sync(chan->vi->dev);
+ if (ret < 0) {
+ dev_err(chan->vi->dev,
+ "failed to get runtime PM: %d\n", ret);
+ pm_runtime_put_noidle(chan->vi->dev);
+ atomic_dec(&chan->vi->power_on_refcnt);
+ return ret;
+ }
+ }
+
+ ret = chan->vi->ops->vi_start_streaming(vq, count);
+
+ if (ret < 0 && atomic_dec_and_test(&chan->vi->power_on_refcnt))
+ pm_runtime_put_sync(chan->vi->dev);
+
+ return ret;
}

static void tegra_channel_stop_streaming(struct vb2_queue *vq)
@@ -209,6 +226,9 @@ static void tegra_channel_stop_streaming(struct vb2_queue *vq)
struct tegra_vi_channel *chan = vb2_get_drv_priv(vq);

chan->vi->ops->vi_stop_streaming(vq);
+
+ if (atomic_dec_and_test(&chan->vi->power_on_refcnt))
+ pm_runtime_put_sync(chan->vi->dev);
}

static const struct vb2_ops tegra_channel_queue_qops = {
@@ -920,13 +940,6 @@ static int tegra_vi_init(struct host1x_client *client)

INIT_LIST_HEAD(&vi->vi_chans);

- ret = pm_runtime_get_sync(vi->dev);
- if (ret < 0) {
- dev_err(vi->dev, "failed to get runtime PM: %d\n", ret);
- pm_runtime_put_noidle(vi->dev);
- return ret;
- }
-
ret = tegra_vi_tpg_channels_alloc(vi);
if (ret < 0) {
dev_err(vi->dev, "failed to allocate tpg channels: %d\n", ret);
@@ -947,14 +960,11 @@ static int tegra_vi_init(struct host1x_client *client)
kfree(chan);
}

- pm_runtime_put_sync(vi->dev);
return ret;
}

static int tegra_vi_exit(struct host1x_client *client)
{
- struct tegra_vi *vi = host1x_client_to_vi(client);
-
/*
* Do not cleanup the channels here as application might still be
* holding video device nodes. Channels cleanup will happen during
@@ -962,8 +972,6 @@ static int tegra_vi_exit(struct host1x_client *client)
* device nodes are released.
*/

- pm_runtime_put_sync(vi->dev);
-
return 0;
}

diff --git a/drivers/staging/media/tegra-video/vi.h b/drivers/staging/media/tegra-video/vi.h
index bf0ed1a..b7a5439 100644
--- a/drivers/staging/media/tegra-video/vi.h
+++ b/drivers/staging/media/tegra-video/vi.h
@@ -54,6 +54,7 @@ struct tegra_vi_soc {
* @soc: pointer to SoC data structure
* @ops: vi operations
* @vi_chans: list head for VI channels
+ * @power_on_refcnt: reference counter for power and clocks on/off
*/
struct tegra_vi {
struct device *dev;
@@ -64,6 +65,7 @@ struct tegra_vi {
const struct tegra_vi_soc *soc;
const struct tegra_vi_ops *ops;
struct list_head vi_chans;
+ atomic_t power_on_refcnt;
};

void tegra_channels_cleanup(struct tegra_vi *vi);
--
2.7.4


2020-04-28 14:01:34

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming

28.04.2020 07:20, Sowjanya Komatineni пишет:
> diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
> index b3dd0c3..29ccdae 100644
> --- a/drivers/staging/media/tegra-video/csi.c
> +++ b/drivers/staging/media/tegra-video/csi.c
> @@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
> struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
> struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
> struct tegra_csi *csi = csi_chan->csi;
> + int ret;
> +
> + if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
> + 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);
> + atomic_dec(&csi->clk_refcnt);
> + return ret;
> + }
> + }
> +
> + ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>
> - return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
> + if ((ret < 0 || !enable) && atomic_dec_and_test(&csi->clk_refcnt))
> + pm_runtime_put_sync(csi->dev);

Runtime PM maintains its own refcount, why these
clk_refcnt/power_on_refcnt are needed?

2020-04-28 14:08:58

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming

28.04.2020 07:20, Sowjanya Komatineni пишет:
> + ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>
> - return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
> + if ((ret < 0 || !enable) && atomic_dec_and_test(&csi->clk_refcnt))
> + pm_runtime_put_sync(csi->dev);

There shouldn't be a need to sync the RPM here, hence just
pm_runtime_put(csi->dev). Same for the VI.

2020-04-28 14:54:35

by Sowjanya Komatineni

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming


On 4/28/20 6:59 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 28.04.2020 07:20, Sowjanya Komatineni пишет:
>> diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
>> index b3dd0c3..29ccdae 100644
>> --- a/drivers/staging/media/tegra-video/csi.c
>> +++ b/drivers/staging/media/tegra-video/csi.c
>> @@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
>> struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
>> struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
>> struct tegra_csi *csi = csi_chan->csi;
>> + int ret;
>> +
>> + if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
>> + 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);
>> + atomic_dec(&csi->clk_refcnt);
>> + return ret;
>> + }
>> + }
>> +
>> + ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>
>> - return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>> + if ((ret < 0 || !enable) && atomic_dec_and_test(&csi->clk_refcnt))
>> + pm_runtime_put_sync(csi->dev);
> Runtime PM maintains its own refcount, why these
> clk_refcnt/power_on_refcnt are needed?

Streaming is per channel and we can't turn power/clocks off while other
channels may still be running.

2020-04-28 15:03:27

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming

28.04.2020 17:51, Sowjanya Komatineni пишет:
>
> On 4/28/20 6:59 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 28.04.2020 07:20, Sowjanya Komatineni пишет:
>>> diff --git a/drivers/staging/media/tegra-video/csi.c
>>> b/drivers/staging/media/tegra-video/csi.c
>>> index b3dd0c3..29ccdae 100644
>>> --- a/drivers/staging/media/tegra-video/csi.c
>>> +++ b/drivers/staging/media/tegra-video/csi.c
>>> @@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct v4l2_subdev
>>> *subdev, int enable)
>>>        struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
>>>        struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
>>>        struct tegra_csi *csi = csi_chan->csi;
>>> +     int ret;
>>> +
>>> +     if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
>>> +             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);
>>> +                     atomic_dec(&csi->clk_refcnt);
>>> +                     return ret;
>>> +             }
>>> +     }
>>> +
>>> +     ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>>
>>> -     return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>> +     if ((ret < 0 || !enable) && atomic_dec_and_test(&csi->clk_refcnt))
>>> +             pm_runtime_put_sync(csi->dev);
>> Runtime PM maintains its own refcount, why these
>> clk_refcnt/power_on_refcnt are needed?
>
> Streaming is per channel and we can't turn power/clocks off while other
> channels may still be running.
>

All channels use the same CSI device. You should remove the custom
refcounting.

BTW, next time you'll really need to do refcounting, use the generic kref.

2020-04-28 15:11:42

by Sowjanya Komatineni

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming


On 4/28/20 7:59 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 28.04.2020 17:51, Sowjanya Komatineni пишет:
>> On 4/28/20 6:59 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 28.04.2020 07:20, Sowjanya Komatineni пишет:
>>>> diff --git a/drivers/staging/media/tegra-video/csi.c
>>>> b/drivers/staging/media/tegra-video/csi.c
>>>> index b3dd0c3..29ccdae 100644
>>>> --- a/drivers/staging/media/tegra-video/csi.c
>>>> +++ b/drivers/staging/media/tegra-video/csi.c
>>>> @@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct v4l2_subdev
>>>> *subdev, int enable)
>>>> struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
>>>> struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
>>>> struct tegra_csi *csi = csi_chan->csi;
>>>> + int ret;
>>>> +
>>>> + if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
>>>> + 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);
>>>> + atomic_dec(&csi->clk_refcnt);
>>>> + return ret;
>>>> + }
>>>> + }
>>>> +
>>>> + ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>>>
>>>> - return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>>> + if ((ret < 0 || !enable) && atomic_dec_and_test(&csi->clk_refcnt))
>>>> + pm_runtime_put_sync(csi->dev);
>>> Runtime PM maintains its own refcount, why these
>>> clk_refcnt/power_on_refcnt are needed?
>> Streaming is per channel and we can't turn power/clocks off while other
>> channels may still be running.
>>
> All channels use the same CSI device. You should remove the custom
> refcounting.
>
> BTW, next time you'll really need to do refcounting, use the generic kref.

Before channel stream we enable power/clocks and after streaming we stop.

So without refcount, on stream stop of any of the channel RPM put turns
power/clock but other channels will still be streaming.

2020-04-28 15:17:42

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming

28.04.2020 18:08, Sowjanya Komatineni пишет:
>
> On 4/28/20 7:59 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 28.04.2020 17:51, Sowjanya Komatineni пишет:
>>> On 4/28/20 6:59 AM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 28.04.2020 07:20, Sowjanya Komatineni пишет:
>>>>> diff --git a/drivers/staging/media/tegra-video/csi.c
>>>>> b/drivers/staging/media/tegra-video/csi.c
>>>>> index b3dd0c3..29ccdae 100644
>>>>> --- a/drivers/staging/media/tegra-video/csi.c
>>>>> +++ b/drivers/staging/media/tegra-video/csi.c
>>>>> @@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct v4l2_subdev
>>>>> *subdev, int enable)
>>>>>         struct tegra_vi_channel *chan =
>>>>> v4l2_get_subdev_hostdata(subdev);
>>>>>         struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
>>>>>         struct tegra_csi *csi = csi_chan->csi;
>>>>> +     int ret;
>>>>> +
>>>>> +     if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
>>>>> +             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);

Why this pm_runtime_put_noidle() is needed? This should be wrong, please
remove it.

>>>>> +                     atomic_dec(&csi->clk_refcnt);
>>>>> +                     return ret;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>>>>
>>>>> -     return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>>>> +     if ((ret < 0 || !enable) &&
>>>>> atomic_dec_and_test(&csi->clk_refcnt))
>>>>> +             pm_runtime_put_sync(csi->dev);
>>>> Runtime PM maintains its own refcount, why these
>>>> clk_refcnt/power_on_refcnt are needed?
>>> Streaming is per channel and we can't turn power/clocks off while other
>>> channels may still be running.
>>>
>> All channels use the same CSI device. You should remove the custom
>> refcounting.
>>
>> BTW, next time you'll really need to do refcounting, use the generic
>> kref.
>
> Before channel stream we enable power/clocks and after streaming we stop.
>
> So without refcount, on stream stop of any of the channel RPM put turns
> power/clock but other channels will still be streaming.
>

Runtime PM uses its own refcounting. Please consult the RPM code and
documentation.

https://elixir.bootlin.com/linux/v5.7-rc3/source/include/linux/pm_runtime.h#L78
https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/base/power/runtime.c#L1079

2020-04-28 15:28:14

by Sowjanya Komatineni

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming


On 4/28/20 8:15 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 28.04.2020 18:08, Sowjanya Komatineni пишет:
>> On 4/28/20 7:59 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 28.04.2020 17:51, Sowjanya Komatineni пишет:
>>>> On 4/28/20 6:59 AM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 28.04.2020 07:20, Sowjanya Komatineni пишет:
>>>>>> diff --git a/drivers/staging/media/tegra-video/csi.c
>>>>>> b/drivers/staging/media/tegra-video/csi.c
>>>>>> index b3dd0c3..29ccdae 100644
>>>>>> --- a/drivers/staging/media/tegra-video/csi.c
>>>>>> +++ b/drivers/staging/media/tegra-video/csi.c
>>>>>> @@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct v4l2_subdev
>>>>>> *subdev, int enable)
>>>>>> struct tegra_vi_channel *chan =
>>>>>> v4l2_get_subdev_hostdata(subdev);
>>>>>> struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
>>>>>> struct tegra_csi *csi = csi_chan->csi;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
>>>>>> + 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);
> Why this pm_runtime_put_noidle() is needed? This should be wrong, please
> remove it.

pm_runtime_get_sync() increments power.usage_count prior to rpm_resume

if rpm_resume fails it does not decrement usage_count.

So to balance count on failure, calling pm_runtime_put_noidle()

>
>>>>>> + atomic_dec(&csi->clk_refcnt);
>>>>>> + return ret;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + ret = csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>>>>>
>>>>>> - return csi->ops->csi_streaming(csi_chan, chan->pg_mode, enable);
>>>>>> + if ((ret < 0 || !enable) &&
>>>>>> atomic_dec_and_test(&csi->clk_refcnt))
>>>>>> + pm_runtime_put_sync(csi->dev);
>>>>> Runtime PM maintains its own refcount, why these
>>>>> clk_refcnt/power_on_refcnt are needed?
>>>> Streaming is per channel and we can't turn power/clocks off while other
>>>> channels may still be running.
>>>>
>>> All channels use the same CSI device. You should remove the custom
>>> refcounting.
>>>
>>> BTW, next time you'll really need to do refcounting, use the generic
>>> kref.
>> Before channel stream we enable power/clocks and after streaming we stop.
>>
>> So without refcount, on stream stop of any of the channel RPM put turns
>> power/clock but other channels will still be streaming.
>>
> Runtime PM uses its own refcounting. Please consult the RPM code and
> documentation.
>
> https://elixir.bootlin.com/linux/v5.7-rc3/source/include/linux/pm_runtime.h#L78
> https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/base/power/runtime.c#L1079
Got it. Will remove refcount for RPM put/get

2020-04-28 16:07:02

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming

28.04.2020 18:22, Sowjanya Komatineni пишет:
>
> On 4/28/20 8:15 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 28.04.2020 18:08, Sowjanya Komatineni пишет:
>>> On 4/28/20 7:59 AM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 28.04.2020 17:51, Sowjanya Komatineni пишет:
>>>>> On 4/28/20 6:59 AM, Dmitry Osipenko wrote:
>>>>>> External email: Use caution opening links or attachments

Is it possible to disable this corporate-email message?

>>>>>>
>>>>>>
>>>>>> 28.04.2020 07:20, Sowjanya Komatineni пишет:
>>>>>>> diff --git a/drivers/staging/media/tegra-video/csi.c
>>>>>>> b/drivers/staging/media/tegra-video/csi.c
>>>>>>> index b3dd0c3..29ccdae 100644
>>>>>>> --- a/drivers/staging/media/tegra-video/csi.c
>>>>>>> +++ b/drivers/staging/media/tegra-video/csi.c
>>>>>>> @@ -272,8 +272,25 @@ static int tegra_csi_s_stream(struct
>>>>>>> v4l2_subdev
>>>>>>> *subdev, int enable)
>>>>>>>          struct tegra_vi_channel *chan =
>>>>>>> v4l2_get_subdev_hostdata(subdev);
>>>>>>>          struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
>>>>>>>          struct tegra_csi *csi = csi_chan->csi;
>>>>>>> +     int ret;
>>>>>>> +
>>>>>>> +     if (enable && atomic_add_return(1, &csi->clk_refcnt) == 1) {
>>>>>>> +             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);
>> Why this pm_runtime_put_noidle() is needed? This should be wrong, please
>> remove it.
>
> pm_runtime_get_sync() increments power.usage_count prior to rpm_resume
>
> if rpm_resume fails it does not decrement usage_count.
>
> So to balance count on failure, calling pm_runtime_put_noidle()

Hmm.. maybe you're right. I'll need to take a more detailed look.

2020-04-30 13:27:22

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [RFC PATCH v1 3/5] media: tegra-video: Move PM runtime handle to streaming

28.04.2020 19:04, Dmitry Osipenko пишет:
...
>>>>>>>> +             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);
>>> Why this pm_runtime_put_noidle() is needed? This should be wrong, please
>>> remove it.
>>
>> pm_runtime_get_sync() increments power.usage_count prior to rpm_resume
>>
>> if rpm_resume fails it does not decrement usage_count.
>>
>> So to balance count on failure, calling pm_runtime_put_noidle()
>
> Hmm.. maybe you're right. I'll need to take a more detailed look.
>

I checked the RPM's use-count after error condition and yours variant is
correct. Perhaps we should start correcting all other Tegra drivers that
do not handle the RPM error properly.