Re: [PATCH v8 11/34] gpu: host1x: Add runtime PM and OPP support

On Tue, 17 Aug 2021 at 16:03, Thierry Reding [off-list ref] wrote:

On Tue, Aug 17, 2021 at 02:04:38PM +0200, Ulf Hansson wrote:

quoted

On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko [off-list ref] wrote:

quoted

Add runtime PM and OPP support to the Host1x driver. It's required for
enabling system-wide DVFS and supporting dynamic power management using
a generic power domain. For the starter we will keep host1x always-on
because dynamic power management require a major refactoring of the driver
code since lot's of code paths will need the RPM handling and we're going
to remove some of these paths in the future. Host1x doesn't consume much
power so it is good enough, we at least need to resume Host1x in order
to initialize the power state.

Tested-by: Peter Geis <redacted> # Ouya T30
Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <redacted> # PAZ00 T20 and TK1 T124
Tested-by: Matt Merhar <redacted> # Ouya T30
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---

[...]

quoted

+
 static int host1x_probe(struct platform_device *pdev)
 {
        struct host1x *host;

@@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev)
        /* set common host1x device data */
        platform_set_drvdata(pdev, host);

+       err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
+       if (err)
+               return err;
+
        host->regs = devm_ioremap_resource(&pdev->dev, regs);
        if (IS_ERR(host->regs))
                return PTR_ERR(host->regs);

@@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev)
                return err;
        }

-       host->rst = devm_reset_control_get(&pdev->dev, "host1x");
-       if (IS_ERR(host->rst)) {
-               err = PTR_ERR(host->rst);
-               dev_err(&pdev->dev, "failed to get reset: %d\n", err);
+       err = host1x_get_resets(host);
+       if (err)
                return err;
-       }

        err = host1x_iommu_init(host);
        if (err < 0) {

@@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev)
                goto iommu_exit;
        }

-       err = clk_prepare_enable(host->clk);
-       if (err < 0) {
-               dev_err(&pdev->dev, "failed to enable clock\n");
-               goto free_channels;
-       }
-
-       err = reset_control_deassert(host->rst);
-       if (err < 0) {
-               dev_err(&pdev->dev, "failed to deassert reset: %d\n", err);
-               goto unprepare_disable;
-       }
-

Removing the clk_prepare_enable() and reset_control_deassert() from
host1x_probe(), might not be a good idea. See more about why, below.

quoted

        err = host1x_syncpt_init(host);
        if (err) {
                dev_err(&pdev->dev, "failed to initialize syncpts\n");
-               goto reset_assert;
+               goto free_channels;
        }

        err = host1x_intr_init(host, syncpt_irq);

@@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev)
                goto deinit_syncpt;
        }

-       host1x_debug_init(host);
+       pm_runtime_enable(&pdev->dev);

-       if (host->info->has_hypervisor)
-               host1x_setup_sid_table(host);
+       /* the driver's code isn't ready yet for the dynamic RPM */
+       err = pm_runtime_resume_and_get(&pdev->dev);

If the driver is being built with the CONFIG_PM Kconfig option being
unset, pm_runtime_resume_and_get() will return 0 to indicate success -
and without calling the ->runtime_resume() callback.
In other words, the clock will remain gated and the reset will not be
deasserted, likely causing the driver to be malfunctioning.

If the driver isn't ever being built with CONFIG_PM unset, feel free
to ignore my above comments.

Otherwise, if it needs to work both with and without CONFIG_PM being
set, you may use the following pattern in host1x_probe() to deploy
runtime PM support:

"Enable the needed resources to probe the device"
pm_runtime_get_noresume()
pm_runtime_set_active()
pm_runtime_enable()

"Before successfully completing probe"
pm_runtime_put()

We made a conscious decision a few years ago to have ARCH_TEGRA select
PM on both 32-bit and 64-bit ARM, specifically to avoid the need to do
this dance (though there are still a few drivers left that do this, I
think).

So I think this should be unnecessary. Unless perhaps if the sysfs PM
controls have any influence on this. As far as I know, as long as the
PM kconfig option is enabled, the sysfs control only influence the
runtime behaviour (i.e. setting the sysfs PM control to "on" is going
to force runtime PM to be resumed) but there's no way to disable
runtime PM altogether via sysfs that would make the above necessary.

Thanks for clarifying! As I said, feel free to ignore my comments then.

For this and the other patches in the series, I assume you only need
to care about whether the driver is a cross SoC driver and used on
other platforms than Tegra then.

Thierry

Kind regards
Uffe