* Re: [PATCH v1 1/1] drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe
2026-04-27 7:58 ` [PATCH v1 1/1] " Svyatoslav Ryhel
@ 2026-04-28 1:52 ` Mikko Perttunen
2026-04-28 4:52 ` Claude review: " Claude Code Review Bot
1 sibling, 0 replies; 5+ messages in thread
From: Mikko Perttunen @ 2026-04-28 1:52 UTC (permalink / raw)
To: Thierry Reding, David Airlie, Simona Vetter, Jonathan Hunter,
Ion Agorria, Svyatoslav Ryhel, Svyatoslav Ryhel
Cc: dri-devel, linux-tegra, linux-kernel
On Monday, April 27, 2026 4:58 PM Svyatoslav Ryhel wrote:
> From: Ion Agorria <ion@agorria.com>
>
> The gr*d_remove() has pm_runtime_disable, this indicates it should be
> paired with pm_runtime_enable in the probe instead of being inside
> gr*d_runtime_resume().
>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/gpu/drm/tegra/gr2d.c | 8 ++++----
> drivers/gpu/drm/tegra/gr3d.c | 8 ++++----
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> index 21f4dd0fa6af..71f092d59d65 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -286,6 +286,10 @@ static int gr2d_probe(struct platform_device *pdev)
> for (i = 0; i < ARRAY_SIZE(gr2d_addr_regs); i++)
> set_bit(gr2d_addr_regs[i], gr2d->addr_regs);
>
> + pm_runtime_enable(dev);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_set_autosuspend_delay(dev, 500);
> +
> return 0;
> }
>
> @@ -367,10 +371,6 @@ static int __maybe_unused gr2d_runtime_resume(struct device *dev)
> goto disable_clk;
> }
>
> - pm_runtime_enable(dev);
> - pm_runtime_use_autosuspend(dev);
> - pm_runtime_set_autosuspend_delay(dev, 500);
> -
> return 0;
>
> disable_clk:
> diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> index 42e9656ab80c..33e88ca4d4c5 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -517,6 +517,10 @@ static int gr3d_probe(struct platform_device *pdev)
> for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
> set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
>
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
> +
> return 0;
> }
>
> @@ -578,10 +582,6 @@ static int __maybe_unused gr3d_runtime_resume(struct device *dev)
> goto disable_clk;
> }
>
> - pm_runtime_enable(dev);
> - pm_runtime_use_autosuspend(dev);
> - pm_runtime_set_autosuspend_delay(dev, 500);
> -
> return 0;
>
> disable_clk:
> --
> 2.51.0
>
>
Oof, looks like I had managed to really bungle this with my earlier
patch. Thanks for fixing it!
FWIW, I've been working on adding some nightly testing for Host1x/
TegraDRM, so hopefully we'll be able to catch such things easier
in the future.
Fixes: 62fa0a985e2c ("drm/tegra: Enable runtime PM during probe")
Acked-by: Mikko Perttunen <mperttunen@nvidia.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Claude review: drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe
2026-04-27 7:58 ` [PATCH v1 1/1] " Svyatoslav Ryhel
2026-04-28 1:52 ` Mikko Perttunen
@ 2026-04-28 4:52 ` Claude Code Review Bot
1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-28 4:52 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue: Wrong ordering of pm_runtime calls (moderate)**
In both gr2d and gr3d, the patch adds:
```c
pm_runtime_enable(dev);
pm_runtime_use_autosuspend(dev);
pm_runtime_set_autosuspend_delay(dev, 500);
```
The standard kernel pattern is the reverse — configure autosuspend *before* enabling runtime PM:
```c
pm_runtime_set_autosuspend_delay(dev, 500);
pm_runtime_use_autosuspend(dev);
pm_runtime_enable(dev);
```
Once `pm_runtime_enable()` is called, the runtime PM subsystem is live and can immediately attempt to suspend the device. If autosuspend isn't configured yet, there's a window where a `pm_runtime_put()` could trigger an immediate suspend rather than the intended 500ms delayed autosuspend. The delay should be set first, then autosuspend enabled, then finally runtime PM enabled.
This ordering issue existed in the original `runtime_resume` code too, but there it was less consequential since the device was already actively resuming. In `probe()`, with PM freshly enabled, it matters more.
**Observation: Placement relative to host1x_client_register (minor)**
The `pm_runtime_enable()` is placed after `host1x_client_register()`. If the host1x framework synchronously invokes `gr*d_init()` during registration, and that callback (or anything it triggers) does `pm_runtime_get_sync()`, the resume callback won't fire because runtime PM isn't enabled yet. In practice this is likely safe because `host1x_client_register` just adds the client to a list, but it's worth considering whether enabling PM before registration would be more robust. That would require adding `pm_runtime_disable()` to the error path for `host1x_client_register()` failure though.
**Observation: Missing Fixes tag**
If this addresses a real bug (runtime PM never properly enabled on first boot without a sleep/resume cycle), a `Fixes:` tag referencing the commit that originally placed `pm_runtime_enable` inside `runtime_resume` would help stable backport triage.
**Style nit: gr3d uses `&pdev->dev` vs gr2d uses `dev`**
In `gr2d_probe`, there's a local `struct device *dev = &pdev->dev;`, so the patch correctly uses `dev`. In `gr3d_probe`, there's no such local variable, so `&pdev->dev` is used. This is consistent with the existing code in each file — no issue, just noting the difference.
**Summary**: The change is correct in principle and fixes a real bug. Please reorder the three calls so `pm_runtime_enable()` comes last (after configuring autosuspend), and consider adding a `Fixes:` tag.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 5+ messages in thread