[PATCH v2 0/4] Tegra Video Decoder driver power management corrections

[PATCH v2 0/4] Tegra Video Decoder driver power management corrections

[Dmitry Osipenko]

Hello,

This small series addresses a Runtime PM issue that was discovered during
of Tegra VI driver reviewing by balancing RPM usage count on RPM resume
failure. Secondly it fixes reboot on some Tegra devices due to bootloader
expecting VDE power partition to be ON at the boot time, which wasn't
happening in case of a warm re-booting (i.e. by PMC resetting).

Changelog:

v2: - Extended the commit's message of the "Balance runtime PM use-count on
      resume failure" patch.

    - Re-send for 5.9 inclusion.

Dmitry Osipenko (4):
  media: staging: tegra-vde: Balance runtime PM use-count on resume
    failure
  media: staging: tegra-vde: Runtime PM is always available on Tegra
  media: staging: tegra-vde: Turn ON power domain on shutdown
  media: staging: tegra-vde: Power-cycle hardware on probe

 drivers/staging/media/tegra-vde/vde.c | 45 +++++++++++++++++----------
 1 file changed, 29 insertions(+), 16 deletions(-)

-- 
2.26.0

[PATCH v2 1/4] media: staging: tegra-vde: Balance runtime PM use-count on resume failure

[Dmitry Osipenko]

The RPM's use-count is getting incremented regardless of the
pm_runtime_get_sync() success or fail. It's up to a driver how to
handle the failed RPM. In the case of VDE driver, the RPM's use-count
should be restored if runtime-resume fails. Use pm_runtime_put_noidle(),
which is the most straight-forward variant to balance the RPM, confirmed
by Rafael J. Wysocki.

Link: https://lore.kernel.org/linux-i2c/CAJZ5v0i87NGcy9+kxubScdPDyByr8ypQWcGgBFn+V-wDd69BHQ@mail.gmail.com/
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/staging/media/tegra-vde/vde.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
index d3e63512a765..803e5dda4bb5 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -776,8 +776,10 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 		goto release_dpb_frames;
 
 	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
 		goto unlock;
+	}
 
 	/*
 	 * We rely on the VDE registers reset value, otherwise VDE
-- 
2.26.0

[PATCH v2 2/4] media: staging: tegra-vde: Runtime PM is always available on Tegra

[Dmitry Osipenko]

Runtime PM is always available on Tegra nowadays since commit 40b2bb1b132a
("ARM: tegra: enforce PM requirement"), hence the case of unavailable RPM
doesn't need to be handled.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/staging/media/tegra-vde/vde.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
index 803e5dda4bb5..85cbbc8f70d3 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -1068,17 +1068,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 300);
 
-	if (!pm_runtime_enabled(dev)) {
-		err = tegra_vde_runtime_resume(dev);
-		if (err)
-			goto err_misc_unreg;
-	}
-
 	return 0;
 
-err_misc_unreg:
-	misc_deregister(&vde->miscdev);
-
 err_deinit_iommu:
 	tegra_vde_iommu_deinit(vde);
 
@@ -1093,13 +1084,6 @@ static int tegra_vde_remove(struct platform_device *pdev)
 {
 	struct tegra_vde *vde = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
-	int err;
-
-	if (!pm_runtime_enabled(dev)) {
-		err = tegra_vde_runtime_suspend(dev);
-		if (err)
-			return err;
-	}
 
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
-- 
2.26.0

[PATCH v2 3/4] media: staging: tegra-vde: Turn ON power domain on shutdown

[Dmitry Osipenko]

On some devices bootloader isn't ready to a clamped VDE power, and thus,
machine hangs on a warm reboot (CPU reset). The VDE power partition is
turned ON by default on a cold boot, hence VDE driver should keep power
partition enabled on system's reboot too. This fixes hang on a warm reboot
on a Tegra20 Acer A500 device, which is handy if Embedded Controller
driver is unavailable, i.e. cold reboot can't be performed.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/staging/media/tegra-vde/vde.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
index 85cbbc8f70d3..b64e35b86fb4 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -1085,9 +1085,17 @@ static int tegra_vde_remove(struct platform_device *pdev)
 	struct tegra_vde *vde = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
 
+	pm_runtime_get_sync(dev);
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 
+	/*
+	 * Balance RPM state, the VDE power domain is left ON and hardware
+	 * is clock-gated. It's safe to reboot machine now.
+	 */
+	pm_runtime_put_noidle(dev);
+	clk_disable_unprepare(vde->clk);
+
 	misc_deregister(&vde->miscdev);
 
 	tegra_vde_dmabuf_cache_unmap_all(vde);
@@ -1099,6 +1107,16 @@ static int tegra_vde_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void tegra_vde_shutdown(struct platform_device *pdev)
+{
+	/*
+	 * On some devices bootloader isn't ready to a power-gated VDE on
+	 * a warm-reboot, machine will hang in that case.
+	 */
+	if (pm_runtime_status_suspended(&pdev->dev))
+		tegra_vde_runtime_resume(&pdev->dev);
+}
+
 static __maybe_unused int tegra_vde_pm_suspend(struct device *dev)
 {
 	struct tegra_vde *vde = dev_get_drvdata(dev);
@@ -1144,6 +1162,7 @@ MODULE_DEVICE_TABLE(of, tegra_vde_of_match);
 static struct platform_driver tegra_vde_driver = {
 	.probe		= tegra_vde_probe,
 	.remove		= tegra_vde_remove,
+	.shutdown	= tegra_vde_shutdown,
 	.driver		= {
 		.name		= "tegra-vde",
 		.of_match_table = tegra_vde_of_match,
-- 
2.26.0

[PATCH v2 4/4] media: staging: tegra-vde: Power-cycle hardware on probe

[Dmitry Osipenko]

VDE partition is left turned ON after bootloader on most devices, hence
let's ensure that it's turned OFF in order to lower power leakage while
hardware is idling by turning it ON and OFF during of the driver's probe.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/staging/media/tegra-vde/vde.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
index b64e35b86fb4..3be96c36bf43 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -1068,6 +1068,14 @@ static int tegra_vde_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 300);
 
+	/*
+	 * VDE partition may be left ON after bootloader, hence let's
+	 * power-cycle it in order to put hardware into a predictable lower
+	 * power state.
+	 */
+	pm_runtime_get_sync(dev);
+	pm_runtime_put(dev);
+
 	return 0;
 
 err_deinit_iommu:
-- 
2.26.0

Re: [PATCH v2 0/4] Tegra Video Decoder driver power management corrections

[Hans Verkuil]

On 24/06/2020 17:08, Dmitry Osipenko wrote:
> Hello,
> 
> This small series addresses a Runtime PM issue that was discovered during
> of Tegra VI driver reviewing by balancing RPM usage count on RPM resume
> failure. Secondly it fixes reboot on some Tegra devices due to bootloader
> expecting VDE power partition to be ON at the boot time, which wasn't
> happening in case of a warm re-booting (i.e. by PMC resetting).

Can you rebase this on top of the media_tree master branch? I think a variant
of patch 1 has already been applied. I found a mail today where you mentioned
that you preferred your version (it looks like I missed that) so you'll need to
rework patch 1.

Sorry about this,

	Hans

> 
> Changelog:
> 
> v2: - Extended the commit's message of the "Balance runtime PM use-count on
>       resume failure" patch.
> 
>     - Re-send for 5.9 inclusion.
> 
> Dmitry Osipenko (4):
>   media: staging: tegra-vde: Balance runtime PM use-count on resume
>     failure
>   media: staging: tegra-vde: Runtime PM is always available on Tegra
>   media: staging: tegra-vde: Turn ON power domain on shutdown
>   media: staging: tegra-vde: Power-cycle hardware on probe
> 
>  drivers/staging/media/tegra-vde/vde.c | 45 +++++++++++++++++----------
>  1 file changed, 29 insertions(+), 16 deletions(-)
> 

Re: [PATCH v2 0/4] Tegra Video Decoder driver power management corrections

[Dmitry Osipenko]

24.06.2020 18:16, Hans Verkuil пишет:
> On 24/06/2020 17:08, Dmitry Osipenko wrote:
>> Hello,
>>
>> This small series addresses a Runtime PM issue that was discovered during
>> of Tegra VI driver reviewing by balancing RPM usage count on RPM resume
>> failure. Secondly it fixes reboot on some Tegra devices due to bootloader
>> expecting VDE power partition to be ON at the boot time, which wasn't
>> happening in case of a warm re-booting (i.e. by PMC resetting).
> 
> Can you rebase this on top of the media_tree master branch? I think a variant
> of patch 1 has already been applied. I found a mail today where you mentioned
> that you preferred your version (it looks like I missed that) so you'll need to
> rework patch 1.

Hello Hans,

I'll take a look at what patches has been applied, my bad for sending
the v2 too late. Thank you for the heads up!

Re: [PATCH v2 0/4] Tegra Video Decoder driver power management corrections

[Dmitry Osipenko]

24.06.2020 18:23, Dmitry Osipenko пишет:
> 24.06.2020 18:16, Hans Verkuil пишет:
>> On 24/06/2020 17:08, Dmitry Osipenko wrote:
>>> Hello,
>>>
>>> This small series addresses a Runtime PM issue that was discovered during
>>> of Tegra VI driver reviewing by balancing RPM usage count on RPM resume
>>> failure. Secondly it fixes reboot on some Tegra devices due to bootloader
>>> expecting VDE power partition to be ON at the boot time, which wasn't
>>> happening in case of a warm re-booting (i.e. by PMC resetting).
>>
>> Can you rebase this on top of the media_tree master branch? I think a variant
>> of patch 1 has already been applied. I found a mail today where you mentioned
>> that you preferred your version (it looks like I missed that) so you'll need to
>> rework patch 1.
> 
> Hello Hans,
> 
> I'll take a look at what patches has been applied, my bad for sending
> the v2 too late. Thank you for the heads up!
> 

I tested the already-applied variant of the patch 1 and it has the same
behaviour as my variant, so it's okay.

Would you want me to send a v3 without the conflicting patch 1 or you
could apply the patches 2-4 from this series?

Re: [PATCH v2 0/4] Tegra Video Decoder driver power management corrections

[Hans Verkuil]

On 26/06/2020 02:17, Dmitry Osipenko wrote:
> 24.06.2020 18:23, Dmitry Osipenko пишет:
>> 24.06.2020 18:16, Hans Verkuil пишет:
>>> On 24/06/2020 17:08, Dmitry Osipenko wrote:
>>>> Hello,
>>>>
>>>> This small series addresses a Runtime PM issue that was discovered during
>>>> of Tegra VI driver reviewing by balancing RPM usage count on RPM resume
>>>> failure. Secondly it fixes reboot on some Tegra devices due to bootloader
>>>> expecting VDE power partition to be ON at the boot time, which wasn't
>>>> happening in case of a warm re-booting (i.e. by PMC resetting).
>>>
>>> Can you rebase this on top of the media_tree master branch? I think a variant
>>> of patch 1 has already been applied. I found a mail today where you mentioned
>>> that you preferred your version (it looks like I missed that) so you'll need to
>>> rework patch 1.
>>
>> Hello Hans,
>>
>> I'll take a look at what patches has been applied, my bad for sending
>> the v2 too late. Thank you for the heads up!
>>
> 
> I tested the already-applied variant of the patch 1 and it has the same
> behaviour as my variant, so it's okay.
> 
> Would you want me to send a v3 without the conflicting patch 1 or you
> could apply the patches 2-4 from this series?
> 

I'll mark 1/4 as superseded and will apply patches 2-4. No need for you
to do anything.

Regards,

	Hans

Re: [PATCH v2 3/4] media: staging: tegra-vde: Turn ON power domain on shutdown

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Wed, Jun 24, 2020 at 06:08:46PM +0300, Dmitry Osipenko wrote:
> On some devices bootloader isn't ready to a clamped VDE power, and thus,
> machine hangs on a warm reboot (CPU reset). The VDE power partition is
> turned ON by default on a cold boot, hence VDE driver should keep power
> partition enabled on system's reboot too. This fixes hang on a warm reboot
> on a Tegra20 Acer A500 device, which is handy if Embedded Controller
> driver is unavailable, i.e. cold reboot can't be performed.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/staging/media/tegra-vde/vde.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Unfortunate that we have to do this, but looks fine:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/4] media: staging: tegra-vde: Runtime PM is always available on Tegra

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Wed, Jun 24, 2020 at 06:08:45PM +0300, Dmitry Osipenko wrote:
> Runtime PM is always available on Tegra nowadays since commit 40b2bb1b132a
> ("ARM: tegra: enforce PM requirement"), hence the case of unavailable RPM
> doesn't need to be handled.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/staging/media/tegra-vde/vde.c | 16 ----------------
>  1 file changed, 16 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/4] media: staging: tegra-vde: Power-cycle hardware on probe

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

On Wed, Jun 24, 2020 at 06:08:47PM +0300, Dmitry Osipenko wrote:
> VDE partition is left turned ON after bootloader on most devices, hence
> let's ensure that it's turned OFF in order to lower power leakage while
> hardware is idling by turning it ON and OFF during of the driver's probe.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/staging/media/tegra-vde/vde.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
> index b64e35b86fb4..3be96c36bf43 100644
> --- a/drivers/staging/media/tegra-vde/vde.c
> +++ b/drivers/staging/media/tegra-vde/vde.c
> @@ -1068,6 +1068,14 @@ static int tegra_vde_probe(struct platform_device *pdev)
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_autosuspend_delay(dev, 300);
>  
> +	/*
> +	 * VDE partition may be left ON after bootloader, hence let's
> +	 * power-cycle it in order to put hardware into a predictable lower
> +	 * power state.
> +	 */
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_put(dev);
> +
>  	return 0;
>  
>  err_deinit_iommu:

Shouldn't this happen automatically? My understanding is that power
domains are turned on automatically before ->probe() and then unless a
runtime PM reference is taken during ->probe() it will get turned off
again after ->probe()?

Is that not happening? Is auto-suspend perhaps getting in the way
somehow?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] Tegra Video Decoder driver power management corrections

[Dmitry Osipenko]

26.06.2020 09:58, Hans Verkuil пишет:
> On 26/06/2020 02:17, Dmitry Osipenko wrote:
>> 24.06.2020 18:23, Dmitry Osipenko пишет:
>>> 24.06.2020 18:16, Hans Verkuil пишет:
>>>> On 24/06/2020 17:08, Dmitry Osipenko wrote:
>>>>> Hello,
>>>>>
>>>>> This small series addresses a Runtime PM issue that was discovered during
>>>>> of Tegra VI driver reviewing by balancing RPM usage count on RPM resume
>>>>> failure. Secondly it fixes reboot on some Tegra devices due to bootloader
>>>>> expecting VDE power partition to be ON at the boot time, which wasn't
>>>>> happening in case of a warm re-booting (i.e. by PMC resetting).
>>>>
>>>> Can you rebase this on top of the media_tree master branch? I think a variant
>>>> of patch 1 has already been applied. I found a mail today where you mentioned
>>>> that you preferred your version (it looks like I missed that) so you'll need to
>>>> rework patch 1.
>>>
>>> Hello Hans,
>>>
>>> I'll take a look at what patches has been applied, my bad for sending
>>> the v2 too late. Thank you for the heads up!
>>>
>>
>> I tested the already-applied variant of the patch 1 and it has the same
>> behaviour as my variant, so it's okay.
>>
>> Would you want me to send a v3 without the conflicting patch 1 or you
>> could apply the patches 2-4 from this series?
>>
> 
> I'll mark 1/4 as superseded and will apply patches 2-4. No need for you
> to do anything.

Thank you!

Re: [PATCH v2 4/4] media: staging: tegra-vde: Power-cycle hardware on probe

[Dmitry Osipenko]

26.06.2020 10:48, Thierry Reding пишет:
> On Wed, Jun 24, 2020 at 06:08:47PM +0300, Dmitry Osipenko wrote:
>> VDE partition is left turned ON after bootloader on most devices, hence
>> let's ensure that it's turned OFF in order to lower power leakage while
>> hardware is idling by turning it ON and OFF during of the driver's probe.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/staging/media/tegra-vde/vde.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
>> index b64e35b86fb4..3be96c36bf43 100644
>> --- a/drivers/staging/media/tegra-vde/vde.c
>> +++ b/drivers/staging/media/tegra-vde/vde.c
>> @@ -1068,6 +1068,14 @@ static int tegra_vde_probe(struct platform_device *pdev)
>>  	pm_runtime_use_autosuspend(dev);
>>  	pm_runtime_set_autosuspend_delay(dev, 300);
>>  
>> +	/*
>> +	 * VDE partition may be left ON after bootloader, hence let's
>> +	 * power-cycle it in order to put hardware into a predictable lower
>> +	 * power state.
>> +	 */
>> +	pm_runtime_get_sync(dev);
>> +	pm_runtime_put(dev);
>> +
>>  	return 0;
>>  
>>  err_deinit_iommu:
> 
> Shouldn't this happen automatically? My understanding is that power
> domains are turned on automatically before ->probe() and then unless a
> runtime PM reference is taken during ->probe() it will get turned off
> again after ->probe()?

Older Tegra SoCs haven't been converted to use the generic power-domain
API and today's VDE driver supports only T20 and T30 SoCs.

> Is that not happening? Is auto-suspend perhaps getting in the way
> somehow?

We're manually toggling the PD using legacy Tegra-PD API in the driver's
RPM callbacks, that's why the RPM needs to be toggled manually as well.

Perhaps this hunk could be removed if older Tergas would get a genpd
support. I guess it shouldn't be difficult to implement the genpd
support, but then there will be a compatibility trouble with older DTs,
so perhaps it's not really worth the effort.