[PATCH v2 0/2] drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[PATCH v2 0/2] drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[Svyatoslav Ryhel]

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().

---
Changes in v2:
- remove pm from gr2d_exit along with pm from gr2d_runtime_resume
- move register map initialize before host1x_client_register
---

Ion Agorria (1):
  drm/tegra: gr2d/gr3d: Contain PM in the gr*d_probe/gr*d_remove

Svyatoslav Ryhel (1):
  drm/tegra: gr2d/gr3d: Initialize address register map before HOST1X
    client is registered

 drivers/gpu/drm/tegra/gr2d.c | 17 +++++++----------
 drivers/gpu/drm/tegra/gr3d.c | 17 +++++++----------
 2 files changed, 14 insertions(+), 20 deletions(-)

-- 
2.51.0

[PATCH v2 1/2] drm/tegra: gr2d/gr3d: Initialize address register map before HOST1X client is registered

[Svyatoslav Ryhel]

The host1x_client_register() function is called just prior to register map
initialization loop, making the device available to userspace. This may
result in userspace attempting to submits a job before the register map is
initialized. Address this by moving register initialization before host1x
client registration.

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..e4148b034af7 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -276,16 +276,16 @@ static int gr2d_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	/* initialize address register map */
+	for (i = 0; i < ARRAY_SIZE(gr2d_addr_regs); i++)
+		set_bit(gr2d_addr_regs[i], gr2d->addr_regs);
+
 	err = host1x_client_register(&gr2d->client.base);
 	if (err < 0) {
 		dev_err(dev, "failed to register host1x client: %d\n", err);
 		return err;
 	}
 
-	/* initialize address register map */
-	for (i = 0; i < ARRAY_SIZE(gr2d_addr_regs); i++)
-		set_bit(gr2d_addr_regs[i], gr2d->addr_regs);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 42e9656ab80c..47b0c6c56bfd 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -506,6 +506,10 @@ static int gr3d_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	/* initialize address register map */
+	for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
+		set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
+
 	err = host1x_client_register(&gr3d->client.base);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to register host1x client: %d\n",
@@ -513,10 +517,6 @@ static int gr3d_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	/* initialize address register map */
-	for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
-		set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
-
 	return 0;
 }
 
-- 
2.51.0

Claude review: drm/tegra: gr2d/gr3d: Initialize address register map before HOST1X client is registered

[Claude Code Review Bot]

Patch Review

**Assessment: Good.**

This patch moves the `addr_regs` bitmap initialization loop before `host1x_client_register()` in both `gr2d_probe()` and `gr3d_probe()`.

The change is correct. The `addr_regs` bitmap is consulted by `gr2d_is_addr_reg()` / `gr3d_is_addr_reg()` during job submission to determine which registers contain buffer addresses and must be validated/relocated. Leaving this uninitialized after registration means an early job submission could bypass address validation — a correctness and potentially security-relevant issue.

**Minor nit on the commit message:** The commit says:

> The host1x_client_register() function is called just prior to register map initialization loop, making the device available to userspace. This may result in userspace attempting to submits a job before the register map is initialized.

The actual path from `host1x_client_register()` to userspace job submission is not direct — it requires the `init` callback to fire (which creates the channel and registers with DRM), then userspace must open the device and submit. So the race window described in the commit message is slightly overstated. In practice, the `init` callback firing synchronously during registration would be needed. That said, the fix is still the right thing to do — initialization should precede registration as a matter of defensive coding.

The diff is clean and mechanical — pure code motion, no behavioral change beyond ordering.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 2/2] drm/tegra: gr2d/gr3d: Contain PM in the gr*d_probe/gr*d_remove

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

The current power management configuration causes GR2G/GR3D to malfunction
after resume. Reconfigure all PM actions to be handled within the GR*D
probe and remove operations to address this.

Fixes: 62fa0a985e2c ("drm/tegra: Enable runtime PM during probe")
Acked-by: Mikko Perttunen <mperttunen@nvidia.com>
Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/gpu/drm/tegra/gr2d.c | 11 ++++-------
 drivers/gpu/drm/tegra/gr3d.c | 11 ++++-------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index e4148b034af7..ffcd076b5831 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -100,9 +100,6 @@ static int gr2d_exit(struct host1x_client *client)
 	if (err < 0)
 		return err;
 
-	pm_runtime_dont_use_autosuspend(client->dev);
-	pm_runtime_force_suspend(client->dev);
-
 	host1x_client_iommu_detach(client);
 	host1x_syncpt_put(client->syncpts[0]);
 	host1x_channel_put(gr2d->channel);
@@ -286,6 +283,10 @@ static int gr2d_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	pm_runtime_enable(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 500);
+
 	return 0;
 }
 
@@ -367,10 +368,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 47b0c6c56bfd..cd5554e2117f 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -109,9 +109,6 @@ static int gr3d_exit(struct host1x_client *client)
 	if (err < 0)
 		return err;
 
-	pm_runtime_dont_use_autosuspend(client->dev);
-	pm_runtime_force_suspend(client->dev);
-
 	host1x_client_iommu_detach(client);
 	host1x_syncpt_put(client->syncpts[0]);
 	host1x_channel_put(gr3d->channel);
@@ -517,6 +514,10 @@ static int gr3d_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
+
 	return 0;
 }
 
@@ -578,10 +579,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

Re: [PATCH v2 2/2] drm/tegra: gr2d/gr3d: Contain PM in the gr*d_probe/gr*d_remove

[Svyatoslav Ryhel]

нд, 3 трав. 2026 р. о 19:38 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> From: Ion Agorria <ion@agorria.com>
>
> The current power management configuration causes GR2G/GR3D to malfunction
> after resume. Reconfigure all PM actions to be handled within the GR*D
> probe and remove operations to address this.
>
> Fixes: 62fa0a985e2c ("drm/tegra: Enable runtime PM during probe")
> Acked-by: Mikko Perttunen <mperttunen@nvidia.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/gpu/drm/tegra/gr2d.c | 11 ++++-------
>  drivers/gpu/drm/tegra/gr3d.c | 11 ++++-------
>  2 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> index e4148b034af7..ffcd076b5831 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -100,9 +100,6 @@ static int gr2d_exit(struct host1x_client *client)
>         if (err < 0)
>                 return err;
>
> -       pm_runtime_dont_use_autosuspend(client->dev);
> -       pm_runtime_force_suspend(client->dev);
> -
>         host1x_client_iommu_detach(client);
>         host1x_syncpt_put(client->syncpts[0]);
>         host1x_channel_put(gr2d->channel);
> @@ -286,6 +283,10 @@ static int gr2d_probe(struct platform_device *pdev)
>                 return err;
>         }
>
> +       pm_runtime_enable(dev);
> +       pm_runtime_use_autosuspend(dev);
> +       pm_runtime_set_autosuspend_delay(dev, 500);
> +

Hello Mikko!

I have used same setup as in VIC. May you please take a look to sashiko's check
https://sashiko.dev/#/patchset/20260502124055.22475-1-clamor95%40gmail.com

I do agree with statement that pm_runtime_enable should be before
host1x_client_register since this same approach is widely used in the
media subsystem too.

But I am more interested in your thoughts regarding sashiko's
gr2d_exit situation reasoning.

Thank you!

>         return 0;
>  }
>
> @@ -367,10 +368,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 47b0c6c56bfd..cd5554e2117f 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -109,9 +109,6 @@ static int gr3d_exit(struct host1x_client *client)
>         if (err < 0)
>                 return err;
>
> -       pm_runtime_dont_use_autosuspend(client->dev);
> -       pm_runtime_force_suspend(client->dev);
> -
>         host1x_client_iommu_detach(client);
>         host1x_syncpt_put(client->syncpts[0]);
>         host1x_channel_put(gr3d->channel);
> @@ -517,6 +514,10 @@ static int gr3d_probe(struct platform_device *pdev)
>                 return err;
>         }
>
> +       pm_runtime_enable(&pdev->dev);
> +       pm_runtime_use_autosuspend(&pdev->dev);
> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
> +
>         return 0;
>  }
>
> @@ -578,10 +579,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
>

Claude review: drm/tegra: gr2d/gr3d: Contain PM in the gr*d_probe/gr*d_remove

[Claude Code Review Bot]

Patch Review

**Assessment: Good, one typo and one observation.**

This patch does three things in both gr2d.c and gr3d.c:

1. Adds `pm_runtime_enable()`, `pm_runtime_use_autosuspend()`, `pm_runtime_set_autosuspend_delay()` to `gr*d_probe()`.
2. Removes the same three calls from `gr*d_runtime_resume()`.
3. Removes `pm_runtime_dont_use_autosuspend()` and `pm_runtime_force_suspend()` from `gr*d_exit()`.

This is the correct standard pattern. The current code has `pm_runtime_enable()` inside the runtime resume callback, which is architecturally circular — the PM core can only call the runtime resume callback if runtime PM is already enabled. On a second system suspend/resume cycle, `pm_runtime_enable()` gets called again with `disable_depth == 0`, triggering a kernel warning and PM state corruption.

**Typo in commit message:**

> The current power management configuration causes GR2G/GR3D to malfunction after resume.

`GR2G` should be `GR2D`.

**Observation — removal of `pm_runtime_force_suspend()` from `exit()`:**

The patch removes these lines from `gr2d_exit()` / `gr3d_exit()`:
```c
pm_runtime_dont_use_autosuspend(client->dev);
pm_runtime_force_suspend(client->dev);
```

This means during driver teardown, the hardware won't be explicitly suspended by `exit()`. The sequence becomes:
1. `remove()`: `pm_runtime_disable()` — disables PM tracking
2. `remove()`: `host1x_client_unregister()` → triggers `exit()` → frees channels/syncpts/IOMMU
3. devm cleanup releases clocks and resets

Since `pm_runtime_disable()` is called before `exit()`, and `gr*d_runtime_suspend()` won't be invoked (PM is disabled), the channel won't be explicitly stopped via `host1x_channel_stop()` that normally runs in `gr*d_runtime_suspend()`. The clocks and resets are devm-managed so they'll be cleaned up automatically, but skipping `host1x_channel_stop()` during teardown is a behavioral change worth being aware of. In practice this is likely fine since all users have been torn down by this point, but it's worth confirming with the Tegra maintainers.

**Error path:** `pm_runtime_enable()` is placed as the last operation before `return 0` in both probe functions, so there's no failure path that needs a corresponding `pm_runtime_disable()` — this is correct.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[Claude Code Review Bot]

Overall Series Review

Subject: drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Patches: 4
Reviewed: 2026-05-05T08:41:52.459997

---

This is a well-motivated 2-patch series fixing power management issues in the Tegra GR2D/GR3D DRM drivers. The series makes two corrections:

1. Reorders address register map initialization before `host1x_client_register()` to close a theoretical race window.
2. Moves `pm_runtime_enable()` and autosuspend configuration from the runtime resume callback to `probe()`, and removes the corresponding PM teardown from `exit()`. This follows the standard Linux PM pattern and fixes a real resume malfunction.

The current upstream code has `pm_runtime_enable()` inside `gr*d_runtime_resume()`, which is architecturally wrong — runtime PM must be enabled before runtime resume can ever be called by the PM core. The only way this ever worked is through the `SET_SYSTEM_SLEEP_PM_OPS` path (`pm_runtime_force_resume`), and it breaks on the second system suspend/resume cycle because `pm_runtime_enable()` gets called with `disable_depth` already at 0, producing an "Unbalanced pm_runtime_enable!" warning and subsequent misbehavior.

Both patches are correct and the series is in a good state. One typo and one minor concern noted below. The Acked-by from Mikko Perttunen on patch 2 is a good sign.

**Series: Looks good with minor nits.**

---
Generated by Claude Code Patch Reviewer

[PATCH v1 0/1] drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[Svyatoslav Ryhel]

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().

Ion Agorria (1):
  drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

 drivers/gpu/drm/tegra/gr2d.c | 8 ++++----
 drivers/gpu/drm/tegra/gr3d.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.51.0

[PATCH v1 1/1] drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[Svyatoslav Ryhel]

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

Claude review: drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[Claude Code Review Bot]

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

Claude review: drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[Claude Code Review Bot]

Overall Series Review

Subject: drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Patches: 3
Reviewed: 2026-04-28T14:52:59.542322

---

This is a single-patch series that moves `pm_runtime_enable()`, `pm_runtime_use_autosuspend()`, and `pm_runtime_set_autosuspend_delay()` from the `gr*d_runtime_resume()` callbacks into `gr*d_probe()` for both gr2d and gr3d Tegra DRM drivers.

The fundamental change is **correct and desirable**. Calling `pm_runtime_enable()` from within a `runtime_resume` callback is wrong for two reasons:
1. It creates a chicken-and-egg problem — runtime PM must be enabled before `runtime_resume` can be invoked through normal paths, so on initial boot (without a prior sleep/resume cycle), runtime PM is never enabled.
2. On each subsequent resume, it calls `pm_runtime_enable()` again, decrementing `disable_depth` below zero, producing a kernel warning about an unbalanced enable.

Moving these calls to `probe()` properly pairs them with the `pm_runtime_disable()` already present in `gr*d_remove()`.

However, there is a **call ordering issue** that should be fixed before merging.

---

---
Generated by Claude Code Patch Reviewer