[PATCH v2 0/8] rework runtime_pm for GPU ioctl access

[PATCH v2 0/8] rework runtime_pm for GPU ioctl access

[Shahyan Soltani]

Historically, the GPU was woken up from sleep anytime userspace
triggered an open system call or utilized an ioctl, even if 
hardware access to the GPU was not required.

This 8-patch series reworks the runtime_pm system for ioctls, jobs, and
open/close calls to allow a few tools such as AMD-SMI to function
without waking up the GPU.

v2:
Refactor patch "drm/amdgpu: use pm_runtime_resume_and_get
amdgpu_gem_create_ioctl" to use a conditional guard instead as suggested by
Kevin and Christian.

Change the descriptions for several patches to fit kernel conventions as
suggested by Christian.

Pierre-Eric Pelloux-Prayer (7):
  drm/amdgpu: use pm_runtime_resume_and_get amdgpu_gem_create_ioctl
  drm/amdgpu: use pm_runtime_resume_and_get for ctx ioctl
  drm/amdgpu: rework runtime pm management for cs
  drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl
  drm/amdgpu: handle runtime pm in amdgpu_gem_userptr_ioctl
  drm/amdgpu: prevent suspend in fault handler
  drm/amdgpu: get rid of amdgpu_drm_ioctl

Shahyan Soltani (1):
  drm/amdgpu: defer GPU resume from open/close to first use

 drivers/gpu/drm/amd/amdgpu/Makefile        |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c    | 10 ++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 23 +----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h    |  6 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 27 +++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c     | 15 +++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c  | 45 ----------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 42 +++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h    |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 16 ++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c  | 12 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  8 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  5 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c  |  2 +-
 15 files changed, 114 insertions(+), 102 deletions(-)
 delete mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c

-- 
2.55.0

[PATCH v2 1/8] drm/amdgpu: use pm_runtime_resume_and_get amdgpu_gem_create_ioctl

[Shahyan Soltani]

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

This ioctl might trigger GPU work through ttm so make sure the GPU
is awake.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Co-developed-by: Shahyan Soltani <shahyan.soltani@amd.com>
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 0d579517c03c..4e4814caf61a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -33,6 +33,7 @@
 #include <linux/dma-buf.h>
 #include <linux/dma-fence-unwrap.h>
 #include <linux/uaccess.h>
+#include <linux/pm_runtime.h>
 
 #include <drm/amdgpu_drm.h>
 #include <drm/drm_drv.h>
@@ -442,6 +443,12 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
 	uint32_t handle, initial_domain;
 	int r;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
+
+	r = PM_RUNTIME_ACQUIRE_ERR(&lock);
+	if (r)
+		return r;
+
 	/* reject invalid gem flags */
 	if (flags & ~AMDGPU_GEM_CREATE_SETTABLE_MASK)
 		return -EINVAL;
-- 
2.55.0

[PATCH v2 2/8] drm/amdgpu: use pm_runtime_resume_and_get for ctx ioctl

[Shahyan Soltani]

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Only amdgpu_ctx_stable_pstate communicates with the hw.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index b15ed4a534f1..a87f26139496 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -28,6 +28,7 @@
 #include "amdgpu_sched.h"
 #include "amdgpu_ras.h"
 #include <linux/nospec.h>
+#include <linux/pm_runtime.h>
 
 #define to_amdgpu_ctx_entity(e)	\
 	container_of((e), struct amdgpu_ctx_entity, entity)
@@ -643,10 +644,15 @@ static int amdgpu_ctx_stable_pstate(struct amdgpu_device *adev,
 	 * context id.
 	 */
 
-	if (set)
+	if (set) {
+		r = pm_runtime_resume_and_get(adev->dev);
+		if (r < 0)
+			return r;
 		r = amdgpu_ctx_set_stable_pstate(ctx, *stable_pstate);
-	else
+		pm_runtime_put_autosuspend(adev->dev);
+	} else {
 		*stable_pstate = amdgpu_get_stable_pstate(adev);
+	}
 
 	amdgpu_ctx_put(ctx);
 	return r;
-- 
2.55.0

[PATCH v2 3/8] drm/amdgpu: rework runtime pm management for cs

[Shahyan Soltani]

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Creating a job will now wake up the GPU. The rpm ref will be dropped
either in amdgpu_ib_schedule (because at this point the amdgpu_fence
took a ref) or in amdgpu_job_free (for unsubmitted jobs).

Jobs submitted directly are a bit tricky because they might be fired
from suspend/resume sequence and for now they're left unmodified.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 15 +++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 42 +++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h |  2 ++
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 0f8b097001e3..3b55dd6ce8a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -27,6 +27,7 @@
  *          Christian König
  */
 #include <linux/seq_file.h>
+#include <linux/pm_runtime.h>
 #include <drm/amdgpu_drm.h>
 
 #include "amdgpu.h"
@@ -297,8 +298,18 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct amdgpu_job *job,
 
 	amdgpu_fence_emit(ring, af, fence_flags);
 	*f = &af->base;
-	/* get a ref for the job */
-	dma_fence_get(*f);
+
+	if (job) {
+		/* get a ref for the job */
+		dma_fence_get(*f);
+		/* now that the fence has been sent to the hw, we can drop
+		 * our pm ref.
+		 */
+		if (job->holds_pm_ref) {
+			pm_runtime_put_autosuspend(adev->dev);
+			job->holds_pm_ref = false;
+		}
+	}
 
 	if (ring->funcs->insert_end)
 		ring->funcs->insert_end(ring);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 0056a93c400a..417f2619dfb0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -24,6 +24,7 @@
 #include <linux/kthread.h>
 #include <linux/wait.h>
 #include <linux/sched.h>
+#include <linux/pm_runtime.h>
 
 #include <drm/drm_drv.h>
 
@@ -201,15 +202,24 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		     unsigned int num_ibs, u64 drm_client_id,
 		     gfp_t gfp_flags, struct amdgpu_job **job)
 {
+	bool take_pm_ref = entity && !adev->in_runpm;
 	struct amdgpu_fence *af;
 	int r;
 
 	if (num_ibs == 0)
 		return -EINVAL;
 
+	if (take_pm_ref) {
+		r = pm_runtime_resume_and_get(adev->dev);
+		if (r < 0)
+			return r;
+	}
+
 	*job = kzalloc_flex(**job, ibs, num_ibs, gfp_flags);
-	if (!*job)
-		return -ENOMEM;
+	if (!*job) {
+		r = -ENOMEM;
+		goto out_pm;
+	}
 
 	af = kzalloc_obj(struct amdgpu_fence, gfp_flags);
 	if (!af) {
@@ -230,6 +240,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	amdgpu_sync_create(&(*job)->explicit_sync);
 	(*job)->generation = amdgpu_vm_generation(adev, vm);
 	(*job)->vm_pd_addr = AMDGPU_BO_INVALID_OFFSET;
+	(*job)->holds_pm_ref = take_pm_ref;
 
 	if (!entity)
 		return 0;
@@ -245,6 +256,9 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 err_job:
 	kfree(*job);
 	*job = NULL;
+out_pm:
+	if (take_pm_ref)
+		pm_runtime_put_autosuspend(adev->dev);
 
 	return r;
 }
@@ -270,6 +284,8 @@ int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
 		kfree((*job)->hw_vm_fence);
 		kfree((*job)->hw_fence);
 		kfree(*job);
+		if ((*job)->holds_pm_ref)
+			pm_runtime_put_autosuspend(adev->dev);
 		*job = NULL;
 	}
 
@@ -314,6 +330,17 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
 static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
 {
 	struct amdgpu_job *job = to_amdgpu_job(s_job);
+	struct amdgpu_ring *ring;
+
+	if (job->holds_pm_ref && job->base.sched) {
+		ring = to_amdgpu_ring(job->base.sched);
+		WARN_ON(!ring);
+
+		/* Should have been cleared on submit, but we have to drop
+		 * it in case the job was cancelled.
+		 */
+		pm_runtime_put_autosuspend(ring->adev->dev);
+	}
 
 	drm_sched_job_cleanup(s_job);
 
@@ -349,9 +376,20 @@ void amdgpu_job_set_gang_leader(struct amdgpu_job *job,
 
 void amdgpu_job_free(struct amdgpu_job *job)
 {
+	struct amdgpu_ring *ring;
+
 	if (job->base.entity)
 		drm_sched_job_cleanup(&job->base);
 
+	if (job->holds_pm_ref && job->base.sched) {
+		ring = to_amdgpu_ring(job->base.sched);
+
+		/* Should have been cleared on submit, but we have to drop
+		 * it in case the job was cancelled.
+		 */
+		pm_runtime_put_autosuspend(ring->adev->dev);
+	}
+
 	amdgpu_job_free_resources(job);
 	amdgpu_sync_free(&job->explicit_sync);
 	if (job->gang_submit != &job->base.s_fence->scheduled)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
index 7e03bd2ff639..e7ab7e38f6df 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
@@ -101,6 +101,8 @@ struct amdgpu_job {
 	bool			enforce_isolation;
 	bool			run_cleaner_shader;
 
+	bool			holds_pm_ref;
+
 	uint32_t		num_ibs;
 	struct amdgpu_ib	ibs[];
 };
-- 
2.55.0

[PATCH v2 4/8] drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl

[Shahyan Soltani]

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

For now use the same logic as the existing one, but it's not 100%
correct since:
1) most operations don't need the GPUs
2) some operations that need it are actually deferred

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 4e4814caf61a..6edd03647c2d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -978,6 +978,14 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	if (r)
 		goto error;
 
+	/* FIXME: only PRT operations use the GPU to update mapping, but
+	 * some operations are deferred to fences callbacks so it's not
+	 * trivial to fix.
+	 */
+	r = pm_runtime_resume_and_get(adev->dev);
+	if (r < 0)
+		return r;
+
 	switch (args->operation) {
 	case AMDGPU_VA_OP_MAP:
 		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
@@ -1031,6 +1039,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 
 	}
 
+	pm_runtime_put_autosuspend(adev->dev);
+
 error:
 	dma_fence_chain_free(timeline_chain);
 	if (timeline_syncobj)
-- 
2.55.0

[PATCH v2 5/8] drm/amdgpu: handle runtime pm in amdgpu_gem_userptr_ioctl

[Shahyan Soltani]

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Wake up the GPU while setting up user pages and validating the buffer.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
v2 (Shahyan): Added commit description
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 6edd03647c2d..e54d30f75b2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -594,11 +594,18 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 		if (r)
 			goto user_pages_done;
 
+		r = pm_runtime_resume_and_get(adev->dev);
+		if (r < 0)
+			goto user_pages_done;
+
 		amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm, range);
 
 		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
 		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
 		amdgpu_bo_unreserve(bo);
+
+		pm_runtime_put_autosuspend(adev->dev);
+
 		if (r)
 			goto user_pages_done;
 	}
-- 
2.55.0

[PATCH v2 6/8] drm/amdgpu: prevent suspend in fault handler

[Shahyan Soltani]

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Temporarily increment the PM reference count during page handling to
ensure the GPU does not enter a sleep state.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
v2 (Shahyan): Added commit description
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index e54d30f75b2e..cc6b7c4a947d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -129,6 +129,8 @@ static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf)
 		return ret;
 
 	if (drm_dev_enter(ddev, &idx)) {
+		pm_runtime_get_noresume(ddev->dev);
+
 		ret = amdgpu_bo_fault_reserve_notify(bo);
 		if (ret) {
 			drm_dev_exit(idx);
@@ -138,6 +140,7 @@ static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf)
 		ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
 					       TTM_BO_VM_NUM_PREFAULT);
 
+		pm_runtime_put_noidle(ddev->dev);
 		drm_dev_exit(idx);
 	} else {
 		ret = ttm_bo_vm_dummy_page(vmf, vmf->vma->vm_page_prot);
-- 
2.55.0

[PATCH v2 7/8] drm/amdgpu: get rid of amdgpu_drm_ioctl

[Shahyan Soltani]

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Remove the entire amdgpu_drm_ioctl handler system in favor of drm_ioctl.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
v2 (Shahyan): Added commit description
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Makefile       |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   | 23 +-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h   |  6 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c | 45 -----------------------
 4 files changed, 2 insertions(+), 73 deletions(-)
 delete mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index 7b08814a24da..d5ddf236a6f9 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -321,7 +321,6 @@ include $(FULL_AMD_PATH)/acp/Makefile
 amdgpu-y += $(AMD_ACP_FILES)
 endif
 
-amdgpu-$(CONFIG_COMPAT) += amdgpu_ioc32.o
 amdgpu-$(CONFIG_VGA_SWITCHEROO) += amdgpu_atpx_handler.o
 amdgpu-$(CONFIG_ACPI) += amdgpu_acpi.o
 amdgpu-$(CONFIG_HMM_MIRROR) += amdgpu_hmm.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 9c5e93cd3ee6..f51f03ae5241 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -3019,25 +3019,6 @@ static int amdgpu_drm_release(struct inode *inode, struct file *filp)
 	return drm_release(inode, filp);
 }
 
-long amdgpu_drm_ioctl(struct file *filp,
-		      unsigned int cmd, unsigned long arg)
-{
-	struct drm_file *file_priv = filp->private_data;
-	struct drm_device *dev;
-	long ret;
-
-	dev = file_priv->minor->dev;
-	ret = pm_runtime_get_sync(dev->dev);
-	if (ret < 0)
-		goto out;
-
-	ret = drm_ioctl(filp, cmd, arg);
-
-out:
-	pm_runtime_put_autosuspend(dev->dev);
-	return ret;
-}
-
 static const struct dev_pm_ops amdgpu_pm_ops = {
 	.prepare = pm_sleep_ptr(amdgpu_pmops_prepare),
 	.complete = pm_sleep_ptr(amdgpu_pmops_complete),
@@ -3070,12 +3051,12 @@ static const struct file_operations amdgpu_driver_kms_fops = {
 	.open = drm_open,
 	.flush = amdgpu_flush,
 	.release = amdgpu_drm_release,
-	.unlocked_ioctl = amdgpu_drm_ioctl,
+	.unlocked_ioctl = drm_ioctl,
 	.mmap = drm_gem_mmap,
 	.poll = drm_poll,
 	.read = drm_read,
 #ifdef CONFIG_COMPAT
-	.compat_ioctl = amdgpu_kms_compat_ioctl,
+	.compat_ioctl = drm_compat_ioctl,
 #endif
 #ifdef CONFIG_PROC_FS
 	.show_fdinfo = drm_show_fdinfo,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h
index 2d86cc6f7f4d..6b06c0956e48 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h
@@ -43,10 +43,4 @@
 
 extern const struct drm_driver amdgpu_partition_driver;
 
-long amdgpu_drm_ioctl(struct file *filp,
-		      unsigned int cmd, unsigned long arg);
-
-long amdgpu_kms_compat_ioctl(struct file *filp,
-			     unsigned int cmd, unsigned long arg);
-
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c
deleted file mode 100644
index a1cbd7c3deb2..000000000000
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c
+++ /dev/null
@@ -1,45 +0,0 @@
-/*
- * \file amdgpu_ioc32.c
- *
- * 32-bit ioctl compatibility routines for the AMDGPU DRM.
- *
- * \author Paul Mackerras <paulus@samba.org>
- *
- * Copyright (C) Paul Mackerras 2005
- * All Rights Reserved.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE AUTHOR BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
- * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- */
-#include <linux/compat.h>
-
-#include <drm/amdgpu_drm.h>
-#include <drm/drm_ioctl.h>
-
-#include "amdgpu_drv.h"
-
-long amdgpu_kms_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
-	unsigned int nr = DRM_IOCTL_NR(cmd);
-
-	if (nr < DRM_COMMAND_BASE)
-		return drm_compat_ioctl(filp, cmd, arg);
-
-	return amdgpu_drm_ioctl(filp, cmd, arg);
-}
-- 
2.55.0

[PATCH v2 8/8] drm/amdgpu: defer GPU resume from open/close to first use

[Shahyan Soltani]

Drop pm_runtime_get_sync() from open/close KMS to prevent unnecessary
resumes. Instead, the PM reference will only be held if the GPU is
currently awake.

Introduce delayed virtual memory hardware initialization through a
powered_init flag to skip page table setup if the GPU is suspended.

If the GPU is suspended, pagetables will instead be initialized upon
first use, such as when a cs_ioctl is called.

Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 16 +++++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c  | 12 +++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  8 ++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  5 ++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c  |  2 +-
 6 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index f2f179421c4d..f40d864ca64e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1483,9 +1483,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 
 	file_priv->driver_priv = NULL;
 
-	r = pm_runtime_get_sync(dev->dev);
-	if (r < 0)
-		goto pm_put;
+	int pm_ref = pm_runtime_get_if_active(dev->dev);
 
 	fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
 	if (unlikely(!fpriv)) {
@@ -1499,7 +1497,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 
 	amdgpu_debugfs_vm_init(file_priv);
 
-	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id);
+	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, pm_ref);
 	if (r)
 		goto error_pasid;
 
@@ -1564,9 +1562,8 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 	kfree(fpriv);
 
 out_suspend:
-pm_put:
-	pm_runtime_put_autosuspend(dev->dev);
-
+	if (pm_ref > 0)
+		pm_runtime_put_autosuspend(dev->dev);
 	return r;
 }
 
@@ -1591,7 +1588,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
 	if (!fpriv)
 		return;
 
-	pm_runtime_get_sync(dev->dev);
+	int pm_ref = pm_runtime_get_if_active(dev->dev);
 
 	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_UVD) != NULL)
 		amdgpu_uvd_free_handles(adev, file_priv);
@@ -1630,7 +1627,8 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
 	kfree(fpriv);
 	file_priv->driver_priv = NULL;
 
-	pm_runtime_put_autosuspend(dev->dev);
+	if (pm_ref > 0)
+		pm_runtime_put_autosuspend(dev->dev);
 }
 
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
index 21a225b0116a..7193d853df91 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
@@ -103,11 +103,13 @@ int amdgpu_seq64_map(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		goto error;
 	}
 
-	r = amdgpu_vm_bo_update(adev, *bo_va, false);
-	if (r) {
-		DRM_ERROR("failed to do vm_bo_update on userq sem\n");
-		amdgpu_vm_bo_del(adev, *bo_va);
-		goto error;
+	if (vm->powered_init) {
+		r = amdgpu_vm_bo_update(adev, *bo_va, false);
+		if (r) {
+			DRM_ERROR("failed to do vm_bo_update on userq sem\n");
+			amdgpu_vm_bo_del(adev, *bo_va);
+			goto error;
+		}
 	}
 
 error:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
index ab564202f550..6022ec6cf1b4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
@@ -4071,7 +4071,7 @@ int amdgpu_ualink_manager_start(struct amdgpu_device *adev)
 {
 	int i, r;
 
-	r = amdgpu_vm_init(adev, &adev->ualink.npa_vm, 0);
+	r = amdgpu_vm_init(adev, &adev->ualink.npa_vm, 0, true);
 	if (r)
 		goto out;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 47cde5459469..a03000db3c36 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2596,7 +2596,7 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
  * 0 for success, error for failure.
  */
 int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-		   int32_t xcp_id)
+		   int32_t xcp_id, int device_active)
 {
 	struct amdgpu_bo *root_bo;
 	struct amdgpu_bo_vm *root;
@@ -2620,6 +2620,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	ttm_lru_bulk_move_init(&vm->lru_bulk_move);
 
+	vm->powered_init = device_active;
 	vm->is_compute_context = false;
 	vm->need_tlb_fence = amdgpu_userq_enabled(&adev->ddev);
 
@@ -2640,7 +2641,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	vm->last_update = dma_fence_get_stub();
 	vm->last_unlocked = dma_fence_get_stub();
 	vm->last_tlb_flush = dma_fence_get_stub();
-	vm->generation = amdgpu_vm_generation(adev, NULL);
 
 	mutex_init(&vm->eviction_lock);
 	vm->evicting = false;
@@ -2663,10 +2663,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		goto error_free_root;
 
-	r = amdgpu_vm_pt_clear(adev, vm, root, false);
-	if (r)
-		goto error_free_root;
-
 	r = amdgpu_vm_create_task_info(vm);
 	if (r)
 		dev_dbg(adev->dev, "Failed to create task info for VM\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 0f6634b74974..b422a5f394a1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -373,6 +373,8 @@ struct amdgpu_vm {
 	bool			is_npa;
 	/* Flag to indicate if VM needs a TLB fence (KFD or KGD) */
 	bool			need_tlb_fence;
+	/* Flag to indicate if VM during initialization holds a runtime reference */
+	bool			powered_init;
 
 	/* Memory partition number, -1 means any partition */
 	int8_t			mem_id;
@@ -429,7 +431,8 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev);
 void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
 
 long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
-int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id);
+int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id,
+		   int device_active);
 int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 int amdgpu_vm_make_npa(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index 1cdf2b854f26..a29127f27329 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -456,7 +456,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	bp.size = amdgpu_vm_pt_size(adev, level);
 	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
 
-	if (!adev->gmc.is_app_apu)
+	if (!adev->gmc.is_app_apu && vm->powered_init)
 		bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
 	else
 		bp.domain = AMDGPU_GEM_DOMAIN_GTT;
-- 
2.55.0

Re: [PATCH v2 2/8] drm/amdgpu: use pm_runtime_resume_and_get for ctx ioctl

[Christian König]

On 9/17/26 21:58, Shahyan Soltani wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> 
> Only amdgpu_ctx_stable_pstate communicates with the hw.

Nit: Typo in the commit message, the function name is amdgpu_ctx_set_stable_pstate().

Christian.

> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index b15ed4a534f1..a87f26139496 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -28,6 +28,7 @@
>  #include "amdgpu_sched.h"
>  #include "amdgpu_ras.h"
>  #include <linux/nospec.h>
> +#include <linux/pm_runtime.h>
>  
>  #define to_amdgpu_ctx_entity(e)	\
>  	container_of((e), struct amdgpu_ctx_entity, entity)
> @@ -643,10 +644,15 @@ static int amdgpu_ctx_stable_pstate(struct amdgpu_device *adev,
>  	 * context id.
>  	 */
>  
> -	if (set)
> +	if (set) {
> +		r = pm_runtime_resume_and_get(adev->dev);
> +		if (r < 0)
> +			return r;
>  		r = amdgpu_ctx_set_stable_pstate(ctx, *stable_pstate);
> -	else
> +		pm_runtime_put_autosuspend(adev->dev);
> +	} else {
>  		*stable_pstate = amdgpu_get_stable_pstate(adev);
> +	}
>  
>  	amdgpu_ctx_put(ctx);
>  	return r;

Re: [PATCH v2 3/8] drm/amdgpu: rework runtime pm management for cs

[Christian König]

On 9/17/26 21:58, Shahyan Soltani wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> 
> Creating a job will now wake up the GPU. The rpm ref will be dropped
> either in amdgpu_ib_schedule (because at this point the amdgpu_fence
> took a ref) or in amdgpu_job_free (for unsubmitted jobs).
> 
> Jobs submitted directly are a bit tricky because they might be fired
> from suspend/resume sequence and for now they're left unmodified.

Well that patch is still quite a mess.

I strongly suggest to just drop it completely and have the CS IOCTL have the necessary runtime_pm calls. See amdgpu_cs_ioctl().

Dropping the runtime_pm at the end of that is ok since autosuspend will wait for the submitted work to complete. We actually don't need to mess with the jobs nor the fences here.

Regards,
Christian.

> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 15 +++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 42 +++++++++++++++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.h |  2 ++
>  3 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 0f8b097001e3..3b55dd6ce8a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -27,6 +27,7 @@
>   *          Christian König
>   */
>  #include <linux/seq_file.h>
> +#include <linux/pm_runtime.h>
>  #include <drm/amdgpu_drm.h>
>  
>  #include "amdgpu.h"
> @@ -297,8 +298,18 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct amdgpu_job *job,
>  
>  	amdgpu_fence_emit(ring, af, fence_flags);
>  	*f = &af->base;
> -	/* get a ref for the job */
> -	dma_fence_get(*f);
> +
> +	if (job) {
> +		/* get a ref for the job */
> +		dma_fence_get(*f);
> +		/* now that the fence has been sent to the hw, we can drop
> +		 * our pm ref.
> +		 */
> +		if (job->holds_pm_ref) {
> +			pm_runtime_put_autosuspend(adev->dev);
> +			job->holds_pm_ref = false;
> +		}
> +	}
>  
>  	if (ring->funcs->insert_end)
>  		ring->funcs->insert_end(ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 0056a93c400a..417f2619dfb0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -24,6 +24,7 @@
>  #include <linux/kthread.h>
>  #include <linux/wait.h>
>  #include <linux/sched.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <drm/drm_drv.h>
>  
> @@ -201,15 +202,24 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  		     unsigned int num_ibs, u64 drm_client_id,
>  		     gfp_t gfp_flags, struct amdgpu_job **job)
>  {
> +	bool take_pm_ref = entity && !adev->in_runpm;
>  	struct amdgpu_fence *af;
>  	int r;
>  
>  	if (num_ibs == 0)
>  		return -EINVAL;
>  
> +	if (take_pm_ref) {
> +		r = pm_runtime_resume_and_get(adev->dev);
> +		if (r < 0)
> +			return r;
> +	}
> +
>  	*job = kzalloc_flex(**job, ibs, num_ibs, gfp_flags);
> -	if (!*job)
> -		return -ENOMEM;
> +	if (!*job) {
> +		r = -ENOMEM;
> +		goto out_pm;
> +	}
>  
>  	af = kzalloc_obj(struct amdgpu_fence, gfp_flags);
>  	if (!af) {
> @@ -230,6 +240,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	amdgpu_sync_create(&(*job)->explicit_sync);
>  	(*job)->generation = amdgpu_vm_generation(adev, vm);
>  	(*job)->vm_pd_addr = AMDGPU_BO_INVALID_OFFSET;
> +	(*job)->holds_pm_ref = take_pm_ref;
>  
>  	if (!entity)
>  		return 0;
> @@ -245,6 +256,9 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  err_job:
>  	kfree(*job);
>  	*job = NULL;
> +out_pm:
> +	if (take_pm_ref)
> +		pm_runtime_put_autosuspend(adev->dev);
>  
>  	return r;
>  }
> @@ -270,6 +284,8 @@ int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
>  		kfree((*job)->hw_vm_fence);
>  		kfree((*job)->hw_fence);
>  		kfree(*job);
> +		if ((*job)->holds_pm_ref)
> +			pm_runtime_put_autosuspend(adev->dev);
>  		*job = NULL;
>  	}
>  
> @@ -314,6 +330,17 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
>  static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
>  {
>  	struct amdgpu_job *job = to_amdgpu_job(s_job);
> +	struct amdgpu_ring *ring;
> +
> +	if (job->holds_pm_ref && job->base.sched) {
> +		ring = to_amdgpu_ring(job->base.sched);
> +		WARN_ON(!ring);
> +
> +		/* Should have been cleared on submit, but we have to drop
> +		 * it in case the job was cancelled.
> +		 */
> +		pm_runtime_put_autosuspend(ring->adev->dev);
> +	}
>  
>  	drm_sched_job_cleanup(s_job);
>  
> @@ -349,9 +376,20 @@ void amdgpu_job_set_gang_leader(struct amdgpu_job *job,
>  
>  void amdgpu_job_free(struct amdgpu_job *job)
>  {
> +	struct amdgpu_ring *ring;
> +
>  	if (job->base.entity)
>  		drm_sched_job_cleanup(&job->base);
>  
> +	if (job->holds_pm_ref && job->base.sched) {
> +		ring = to_amdgpu_ring(job->base.sched);
> +
> +		/* Should have been cleared on submit, but we have to drop
> +		 * it in case the job was cancelled.
> +		 */
> +		pm_runtime_put_autosuspend(ring->adev->dev);
> +	}
> +
>  	amdgpu_job_free_resources(job);
>  	amdgpu_sync_free(&job->explicit_sync);
>  	if (job->gang_submit != &job->base.s_fence->scheduled)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> index 7e03bd2ff639..e7ab7e38f6df 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> @@ -101,6 +101,8 @@ struct amdgpu_job {
>  	bool			enforce_isolation;
>  	bool			run_cleaner_shader;
>  
> +	bool			holds_pm_ref;
> +
>  	uint32_t		num_ibs;
>  	struct amdgpu_ib	ibs[];
>  };

Re: [PATCH v2 4/8] drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl

[Christian König]

On 9/17/26 21:58, Shahyan Soltani wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> 
> For now use the same logic as the existing one, but it's not 100%
> correct since:
> 1) most operations don't need the GPUs
> 2) some operations that need it are actually deferred
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 4e4814caf61a..6edd03647c2d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -978,6 +978,14 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  	if (r)
>  		goto error;
>  
> +	/* FIXME: only PRT operations use the GPU to update mapping, but
> +	 * some operations are deferred to fences callbacks so it's not
> +	 * trivial to fix.
> +	 */

That comment is not correct, just drop it.

> +	r = pm_runtime_resume_and_get(adev->dev);
> +	if (r < 0)
> +		return r;
> +
>  	switch (args->operation) {
>  	case AMDGPU_VA_OP_MAP:
>  		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
> @@ -1031,6 +1039,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  
>  	}
>  
> +	pm_runtime_put_autosuspend(adev->dev);

That error handling is not correct, after the switch there are a bunch of returns and gotos for cleanup.

I strongly suggest to use the guard approach here as well.

Regards,
Christian.

> +
>  error:
>  	dma_fence_chain_free(timeline_chain);
>  	if (timeline_syncobj)

Re: [PATCH v2 1/8] drm/amdgpu: use pm_runtime_resume_and_get amdgpu_gem_create_ioctl

[Christian König]

On 9/17/26 21:58, Shahyan Soltani wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> 
> This ioctl might trigger GPU work through ttm so make sure the GPU
> is awake.
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> Co-developed-by: Shahyan Soltani <shahyan.soltani@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

That looks really nice now, good work.

Regards,
Christian.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 0d579517c03c..4e4814caf61a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -33,6 +33,7 @@
>  #include <linux/dma-buf.h>
>  #include <linux/dma-fence-unwrap.h>
>  #include <linux/uaccess.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <drm/amdgpu_drm.h>
>  #include <drm/drm_drv.h>
> @@ -442,6 +443,12 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>  	uint32_t handle, initial_domain;
>  	int r;
>  
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
> +
> +	r = PM_RUNTIME_ACQUIRE_ERR(&lock);
> +	if (r)
> +		return r;
> +
>  	/* reject invalid gem flags */
>  	if (flags & ~AMDGPU_GEM_CREATE_SETTABLE_MASK)
>  		return -EINVAL;

Re: [PATCH v2 5/8] drm/amdgpu: handle runtime pm in amdgpu_gem_userptr_ioctl

[Christian König]

On 9/17/26 21:58, Shahyan Soltani wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> 
> Wake up the GPU while setting up user pages and validating the buffer.
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> v2 (Shahyan): Added commit description
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 6edd03647c2d..e54d30f75b2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -594,11 +594,18 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>  		if (r)
>  			goto user_pages_done;
>  
> +		r = pm_runtime_resume_and_get(adev->dev);

That needs to come before the BO is reserved otherwise we can deadlock.

Apart from that looks good to me,
Christian.

> +		if (r < 0)
> +			goto user_pages_done;
> +
>  		amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm, range);
>  
>  		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
>  		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>  		amdgpu_bo_unreserve(bo);
> +
> +		pm_runtime_put_autosuspend(adev->dev);
> +
>  		if (r)
>  			goto user_pages_done;
>  	}

Re: [PATCH v2 6/8] drm/amdgpu: prevent suspend in fault handler

[Christian König]

On 9/17/26 21:58, Shahyan Soltani wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> 
> Temporarily increment the PM reference count during page handling to
> ensure the GPU does not enter a sleep state.
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> v2 (Shahyan): Added commit description
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index e54d30f75b2e..cc6b7c4a947d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -129,6 +129,8 @@ static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf)
>  		return ret;
>  
>  	if (drm_dev_enter(ddev, &idx)) {
> +		pm_runtime_get_noresume(ddev->dev);
> +
>  		ret = amdgpu_bo_fault_reserve_notify(bo);
>  		if (ret) {
>  			drm_dev_exit(idx);
> @@ -138,6 +140,7 @@ static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf)
>  		ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
>  					       TTM_BO_VM_NUM_PREFAULT);
>  
> +		pm_runtime_put_noidle(ddev->dev);
>  		drm_dev_exit(idx);
>  	} else {
>  		ret = ttm_bo_vm_dummy_page(vmf, vmf->vma->vm_page_prot);

Re: [PATCH v2 8/8] drm/amdgpu: defer GPU resume from open/close to first use

[Christian König]

On 9/17/26 21:58, Shahyan Soltani wrote:
> Drop pm_runtime_get_sync() from open/close KMS to prevent unnecessary
> resumes. Instead, the PM reference will only be held if the GPU is
> currently awake.
> 
> Introduce delayed virtual memory hardware initialization through a
> powered_init flag to skip page table setup if the GPU is suspended.
> 
> If the GPU is suspended, pagetables will instead be initialized upon
> first use, such as when a cs_ioctl is called.
> 
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 16 +++++++---------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c  | 12 +++++++-----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  8 ++------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  5 ++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c  |  2 +-
>  6 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index f2f179421c4d..f40d864ca64e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1483,9 +1483,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  
>  	file_priv->driver_priv = NULL;
>  
> -	r = pm_runtime_get_sync(dev->dev);
> -	if (r < 0)
> -		goto pm_put;
> +	int pm_ref = pm_runtime_get_if_active(dev->dev);
>  
>  	fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
>  	if (unlikely(!fpriv)) {
> @@ -1499,7 +1497,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  
>  	amdgpu_debugfs_vm_init(file_priv);
>  
> -	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id);
> +	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, pm_ref);

Use !!pm_ref as parameter here to convert the int to a bool.

>  	if (r)
>  		goto error_pasid;
>  
> @@ -1564,9 +1562,8 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	kfree(fpriv);
>  
>  out_suspend:
> -pm_put:
> -	pm_runtime_put_autosuspend(dev->dev);
> -
> +	if (pm_ref > 0)
> +		pm_runtime_put_autosuspend(dev->dev);
>  	return r;
>  }
>  
> @@ -1591,7 +1588,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  	if (!fpriv)
>  		return;
>  
> -	pm_runtime_get_sync(dev->dev);
> +	int pm_ref = pm_runtime_get_if_active(dev->dev);
>  
>  	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_UVD) != NULL)
>  		amdgpu_uvd_free_handles(adev, file_priv);
> @@ -1630,7 +1627,8 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  	kfree(fpriv);
>  	file_priv->driver_priv = NULL;
>  
> -	pm_runtime_put_autosuspend(dev->dev);
> +	if (pm_ref > 0)
> +		pm_runtime_put_autosuspend(dev->dev);
>  }
>  
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> index 21a225b0116a..7193d853df91 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> @@ -103,11 +103,13 @@ int amdgpu_seq64_map(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  		goto error;
>  	}
>  
> -	r = amdgpu_vm_bo_update(adev, *bo_va, false);
> -	if (r) {
> -		DRM_ERROR("failed to do vm_bo_update on userq sem\n");
> -		amdgpu_vm_bo_del(adev, *bo_va);
> -		goto error;
> +	if (vm->powered_init) {
> +		r = amdgpu_vm_bo_update(adev, *bo_va, false);
> +		if (r) {
> +			DRM_ERROR("failed to do vm_bo_update on userq sem\n");
> +			amdgpu_vm_bo_del(adev, *bo_va);
> +			goto error;
> +		}

Oh, that is the wrong place for that anyway.

I suggest to have a separate patch to move the call to amdgpu_vm_bo_update() to amdgpu_cs_vm_handling().

>  	}
>  
>  error:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> index ab564202f550..6022ec6cf1b4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> @@ -4071,7 +4071,7 @@ int amdgpu_ualink_manager_start(struct amdgpu_device *adev)
>  {
>  	int i, r;
>  
> -	r = amdgpu_vm_init(adev, &adev->ualink.npa_vm, 0);
> +	r = amdgpu_vm_init(adev, &adev->ualink.npa_vm, 0, true);
>  	if (r)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 47cde5459469..a03000db3c36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2596,7 +2596,7 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
>   * 0 for success, error for failure.
>   */
>  int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -		   int32_t xcp_id)
> +		   int32_t xcp_id, int device_active)
>  {
>  	struct amdgpu_bo *root_bo;
>  	struct amdgpu_bo_vm *root;
> @@ -2620,6 +2620,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  
>  	ttm_lru_bulk_move_init(&vm->lru_bulk_move);
>  
> +	vm->powered_init = device_active;
>  	vm->is_compute_context = false;
>  	vm->need_tlb_fence = amdgpu_userq_enabled(&adev->ddev);
>  
> @@ -2640,7 +2641,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	vm->last_update = dma_fence_get_stub();
>  	vm->last_unlocked = dma_fence_get_stub();
>  	vm->last_tlb_flush = dma_fence_get_stub();

> -	vm->generation = amdgpu_vm_generation(adev, NULL);
>  
>  	mutex_init(&vm->eviction_lock);
>  	vm->evicting = false;
> @@ -2663,10 +2663,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	if (r)
>  		goto error_free_root;
>  

> -	r = amdgpu_vm_pt_clear(adev, vm, root, false);
> -	if (r)
> -		goto error_free_root;
> -

Please move dropping the vm->generation initialization and root PD clearing into a separate patch.

We also need some kind of test that this actually works correctly but that is a much bigger task.

>  	r = amdgpu_vm_create_task_info(vm);
>  	if (r)
>  		dev_dbg(adev->dev, "Failed to create task info for VM\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 0f6634b74974..b422a5f394a1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -373,6 +373,8 @@ struct amdgpu_vm {
>  	bool			is_npa;
>  	/* Flag to indicate if VM needs a TLB fence (KFD or KGD) */
>  	bool			need_tlb_fence;
> +	/* Flag to indicate if VM during initialization holds a runtime reference */
> +	bool			powered_init;
>  
>  	/* Memory partition number, -1 means any partition */
>  	int8_t			mem_id;
> @@ -429,7 +431,8 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev);
>  void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>  
>  long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
> -int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id);
> +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id,
> +		   int device_active);
>  int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>  int amdgpu_vm_make_npa(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>  void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index 1cdf2b854f26..a29127f27329 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -456,7 +456,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	bp.size = amdgpu_vm_pt_size(adev, level);
>  	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
>  
> -	if (!adev->gmc.is_app_apu)
> +	if (!adev->gmc.is_app_apu && vm->powered_init)

That won't work this easily.

This not only affects the initial domain where things are allocated on creation, but also the long term placement.

You need some construct to set bp.domain initially to AMDGPU_GEM_DOMAIN_GTT and then update bo->preffered_domains and bo->allowed_domains after the root PD is created.

Regards,
Christian.

>  		bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
>  	else
>  		bp.domain = AMDGPU_GEM_DOMAIN_GTT;

Re: [PATCH v2 4/8] drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl

[Shahyan Soltani]

On 9/18/26 3:57 AM, Christian König wrote:

>
> On 9/17/26 21:58, Shahyan Soltani wrote:
>> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>
>> For now use the same logic as the existing one, but it's not 100%
>> correct since:
>> 1) most operations don't need the GPUs
>> 2) some operations that need it are actually deferred
>>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 4e4814caf61a..6edd03647c2d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -978,6 +978,14 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>   	if (r)
>>   		goto error;
>>   
>> +	/* FIXME: only PRT operations use the GPU to update mapping, but
>> +	 * some operations are deferred to fences callbacks so it's not
>> +	 * trivial to fix.
>> +	 */
> That comment is not correct, just drop it.
>
>> +	r = pm_runtime_resume_and_get(adev->dev);
>> +	if (r < 0)
>> +		return r;
>> +
>>   	switch (args->operation) {
>>   	case AMDGPU_VA_OP_MAP:
>>   		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>> @@ -1031,6 +1039,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>   
>>   	}
>>   
>> +	pm_runtime_put_autosuspend(adev->dev);
> That error handling is not correct, after the switch there are a bunch of returns and gotos for cleanup.
>
> I strongly suggest to use the guard approach here as well.
>
> Regards,
> Christian.

Thanks for the feedback, you caught something I completely missed. 
However I have a few concerns
on what approaches I could take.

Attempting to use a guard in this situation would require the guard to 
be moved to the top of the function,
resuming the system much early in the stack.

As far as I understand this is due to the guard not being able to 
succeed any gotos as the "lock" variable
in the guard must be initialized before any subsequent goto.

For example:

goto error;
PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);

Is invalid as "lock" is initialized after the goto.
Trying to initialize lock beforehand doesn't work as it would count as a 
redefinition of lock instead.

Would that still be preferred over creating a new label and trying 
something like this instead?

replace

+	r = pm_runtime_resume_and_get(adev->dev);
+	if (r < 0)
+		return r;

with

+	r = pm_runtime_resume_and_get(adev->dev);
+	if (r < 0)
+		goto newlabel;

	}

...

+ newlabel:
+	pm_runtime_put_autosuspend(adev->dev);
error:
	dma_fence_chain_free(timeline_chain);
	if (timeline_syncobj)
		drm_syncobj_put(timeline_syncobj);
	drm_exec_fini(&exec);
error_put_gobj:
	drm_gem_object_put(gobj);
	return r;
}

Another approach I could try is wrap the entirePM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); and the 
subsequent switch cases before the error handling into its own scope 
with {} However, I feel this method is a lot more hacky then necessary. 
int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, struct 
drm_file *filp) { ... + { + 
PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); + + r = 
PM_RUNTIME_ACQUIRE_ERR(&lock); + if (r) + goto error; switch 
(args->operation) { case AMDGPU_VA_OP_MAP: ... case AMDGPU_VA_OP_UNMAP: 
.. case AMDGPU_VA_OP_CLEAR: ... case AMDGPU_VA_OP_REPLACE: ... default: 
... } if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) && 
(!adev->debug_vm || timeline_syncobj)) { ... } + } error: ... 
error_put_gobj: ... } If you have any ideas on what approach I 
should/could take it would be very much appreciated. Thanks, Shahyan

>> +
>>   error:
>>   	dma_fence_chain_free(timeline_chain);
>>   	if (timeline_syncobj)

Re: [PATCH v2 4/8] drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl

[Shahyan Soltani]

On 9/21/26 10:40 AM, Shahyan Soltani wrote:
> On 9/18/26 3:57 AM, Christian König wrote:
>
>>
>> On 9/17/26 21:58, Shahyan Soltani wrote:
>>> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>>
>>> For now use the same logic as the existing one, but it's not 100%
>>> correct since:
>>> 1) most operations don't need the GPUs
>>> 2) some operations that need it are actually deferred
>>>
>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>> <pierre-eric.pelloux-prayer@amd.com>
>>> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 4e4814caf61a..6edd03647c2d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -978,6 +978,14 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, 
>>> void *data,
>>>       if (r)
>>>           goto error;
>>>   +    /* FIXME: only PRT operations use the GPU to update mapping, but
>>> +     * some operations are deferred to fences callbacks so it's not
>>> +     * trivial to fix.
>>> +     */
>> That comment is not correct, just drop it.
>>
>>> +    r = pm_runtime_resume_and_get(adev->dev);
>>> +    if (r < 0)
>>> +        return r;
>>> +
>>>       switch (args->operation) {
>>>       case AMDGPU_VA_OP_MAP:
>>>           r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>> @@ -1031,6 +1039,8 @@ int amdgpu_gem_va_ioctl(struct drm_device 
>>> *dev, void *data,
>>>         }
>>>   +    pm_runtime_put_autosuspend(adev->dev);
>> That error handling is not correct, after the switch there are a 
>> bunch of returns and gotos for cleanup.
>>
>> I strongly suggest to use the guard approach here as well.
>>
>> Regards,
>> Christian.
>
> Thanks for the feedback, you caught something I completely missed. 
> However I have a few concerns
> on what approaches I could take.
>
> Attempting to use a guard in this situation would require the guard to 
> be moved to the top of the function,
> resuming the system much early in the stack.
>
> As far as I understand this is due to the guard not being able to 
> succeed any gotos as the "lock" variable
> in the guard must be initialized before any subsequent goto.
>
> For example:
>
> goto error;
> PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
>
> Is invalid as "lock" is initialized after the goto.
> Trying to initialize lock beforehand doesn't work as it would count as 
> a redefinition of lock instead.
>
> Would that still be preferred over creating a new label and trying 
> something like this instead?
>
> replace
>
> +    r = pm_runtime_resume_and_get(adev->dev);
> +    if (r < 0)
> +        return r;
>
> with
>
> +    r = pm_runtime_resume_and_get(adev->dev);
> +    if (r < 0)
> +        goto newlabel;
>
>     }
>
> ...
>
> + newlabel:
> +    pm_runtime_put_autosuspend(adev->dev);
> error:
>     dma_fence_chain_free(timeline_chain);
>     if (timeline_syncobj)
>         drm_syncobj_put(timeline_syncobj);
>     drm_exec_fini(&exec);
> error_put_gobj:
>     drm_gem_object_put(gobj);
>     return r;
> }
>
> Another approach I could try is wrap the 
> entirePM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); and 
> the subsequent switch cases before the error handling into its own 
> scope with {} However, I feel this method is a lot more hacky then 
> necessary. int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, 
> struct drm_file *filp) { ... + { + 
> PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); + + r = 
> PM_RUNTIME_ACQUIRE_ERR(&lock); + if (r) + goto error; switch 
> (args->operation) { case AMDGPU_VA_OP_MAP: ... case 
> AMDGPU_VA_OP_UNMAP: .. case AMDGPU_VA_OP_CLEAR: ... case 
> AMDGPU_VA_OP_REPLACE: ... default: ... } if (!r && !(args->flags & 
> AMDGPU_VM_DELAY_UPDATE) && (!adev->debug_vm || timeline_syncobj)) { 
> ... } + } error: ... error_put_gobj: ... } If you have any ideas on 
> what approach I should/could take it would be very much appreciated. 
> Thanks, Shahyan
>
I'm not entirely sure what happened here...

But I meant to say wrapping the guard and the subsequent case handling 
into its own scope.

For example:

int amdgpu_gem_va_ioctl() {
...
     goto error;

+    {
+        PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
+        r = PM_RUNTIME_ACQUIRE_ERR(&lock);
+        if (r)
+            goto error;

...
+    }
error:
...
error_put_gobj:
...
}

Thanks,
Shahyan

>>> +
>>>   error:
>>>       dma_fence_chain_free(timeline_chain);
>>>       if (timeline_syncobj)

Re: [PATCH v2 4/8] drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl

[Christian König]

On 9/21/26 16:50, Shahyan Soltani wrote:
> 
> On 9/21/26 10:40 AM, Shahyan Soltani wrote:
>> On 9/18/26 3:57 AM, Christian König wrote:
>>
>>>
>>> On 9/17/26 21:58, Shahyan Soltani wrote:
>>>> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>>>
>>>> For now use the same logic as the existing one, but it's not 100%
>>>> correct since:
>>>> 1) most operations don't need the GPUs
>>>> 2) some operations that need it are actually deferred
>>>>
>>>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>>> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index 4e4814caf61a..6edd03647c2d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -978,6 +978,14 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>       if (r)
>>>>           goto error;
>>>>   +    /* FIXME: only PRT operations use the GPU to update mapping, but
>>>> +     * some operations are deferred to fences callbacks so it's not
>>>> +     * trivial to fix.
>>>> +     */
>>> That comment is not correct, just drop it.
>>>
>>>> +    r = pm_runtime_resume_and_get(adev->dev);
>>>> +    if (r < 0)
>>>> +        return r;
>>>> +
>>>>       switch (args->operation) {
>>>>       case AMDGPU_VA_OP_MAP:
>>>>           r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>> @@ -1031,6 +1039,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>         }
>>>>   +    pm_runtime_put_autosuspend(adev->dev);
>>> That error handling is not correct, after the switch there are a bunch of returns and gotos for cleanup.
>>>
>>> I strongly suggest to use the guard approach here as well.
>>>
>>> Regards,
>>> Christian.
>>
>> Thanks for the feedback, you caught something I completely missed. However I have a few concerns
>> on what approaches I could take.
>>
>> Attempting to use a guard in this situation would require the guard to be moved to the top of the function,
>> resuming the system much early in the stack.
>>
>> As far as I understand this is due to the guard not being able to succeed any gotos as the "lock" variable
>> in the guard must be initialized before any subsequent goto.
>>
>> For example:
>>
>> goto error;
>> PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);

Oh, that is a very good point.

>>
>> Is invalid as "lock" is initialized after the goto.
>> Trying to initialize lock beforehand doesn't work as it would count as a redefinition of lock instead.
>>
>> Would that still be preferred over creating a new label and trying something like this instead?

Yes, as far as I can see the PM reference just needs to be moved before the first goto, right?

When I now check the code, the first goto in function amdgpu_gem_va_ioctl() is that here:

        /* Add input syncobj fences (if any) for synchronization. */
        r = amdgpu_gem_add_input_fence(filp,
                                       args->input_fence_syncobj_handles,
                                       args->num_syncobj_handles);
        if (r)
                goto error_put_gobj;

        drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT |
                      DRM_EXEC_IGNORE_DUPLICATES, 0);


We can't grab the PM reference after drm_exec_init() anyway because that could deadlock.

So that should still work as far as I can see, or do I miss anything?

Regards,
Christian.


>>
>> replace
>>
>> +    r = pm_runtime_resume_and_get(adev->dev);
>> +    if (r < 0)
>> +        return r;
>>
>> with
>>
>> +    r = pm_runtime_resume_and_get(adev->dev);
>> +    if (r < 0)
>> +        goto newlabel;
>>
>>     }
>>
>> ...
>>
>> + newlabel:
>> +    pm_runtime_put_autosuspend(adev->dev);
>> error:
>>     dma_fence_chain_free(timeline_chain);
>>     if (timeline_syncobj)
>>         drm_syncobj_put(timeline_syncobj);
>>     drm_exec_fini(&exec);
>> error_put_gobj:
>>     drm_gem_object_put(gobj);
>>     return r;
>> }
>>
>> Another approach I could try is wrap the entirePM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); and the subsequent switch cases before the error handling into its own scope with {} However, I feel this method is a lot more hacky then necessary. int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) { ... + { + PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); + + r = PM_RUNTIME_ACQUIRE_ERR(&lock); + if (r) + goto error; switch (args->operation) { case AMDGPU_VA_OP_MAP: ... case AMDGPU_VA_OP_UNMAP: .. case AMDGPU_VA_OP_CLEAR: ... case AMDGPU_VA_OP_REPLACE: ... default: ... } if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) && (!adev->debug_vm || timeline_syncobj)) { ... } + } error: ... error_put_gobj: ... } If you have any ideas on what approach I should/could take it would be very much appreciated. Thanks, Shahyan
>>
> I'm not entirely sure what happened here...
> 
> But I meant to say wrapping the guard and the subsequent case handling into its own scope.
> 
> For example:
> 
> int amdgpu_gem_va_ioctl() {
> ...
>     goto error;
> 
> +    {
> +        PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
> +        r = PM_RUNTIME_ACQUIRE_ERR(&lock);
> +        if (r)
> +            goto error;
> 
> ...
> +    }
> error:
> ...
> error_put_gobj:
> ...
> }
> 
> Thanks,
> Shahyan
> 
>>>> +
>>>>   error:
>>>>       dma_fence_chain_free(timeline_chain);
>>>>       if (timeline_syncobj)

Re: [PATCH v2 4/8] drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl

[Shahyan Soltani]

On 9/21/26 11:06 AM, Christian König wrote:
> On 9/21/26 16:50, Shahyan Soltani wrote:
>> On 9/21/26 10:40 AM, Shahyan Soltani wrote:
>>> On 9/18/26 3:57 AM, Christian König wrote:
>>>
>>>> On 9/17/26 21:58, Shahyan Soltani wrote:
>>>>> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>>>>
>>>>> For now use the same logic as the existing one, but it's not 100%
>>>>> correct since:
>>>>> 1) most operations don't need the GPUs
>>>>> 2) some operations that need it are actually deferred
>>>>>
>>>>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>>>> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 ++++++++++
>>>>>    1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 4e4814caf61a..6edd03647c2d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -978,6 +978,14 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>>        if (r)
>>>>>            goto error;
>>>>>    +    /* FIXME: only PRT operations use the GPU to update mapping, but
>>>>> +     * some operations are deferred to fences callbacks so it's not
>>>>> +     * trivial to fix.
>>>>> +     */
>>>> That comment is not correct, just drop it.
>>>>
>>>>> +    r = pm_runtime_resume_and_get(adev->dev);
>>>>> +    if (r < 0)
>>>>> +        return r;
>>>>> +
>>>>>        switch (args->operation) {
>>>>>        case AMDGPU_VA_OP_MAP:
>>>>>            r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>>> @@ -1031,6 +1039,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>>          }
>>>>>    +    pm_runtime_put_autosuspend(adev->dev);
>>>> That error handling is not correct, after the switch there are a bunch of returns and gotos for cleanup.
>>>>
>>>> I strongly suggest to use the guard approach here as well.
>>>>
>>>> Regards,
>>>> Christian.
>>> Thanks for the feedback, you caught something I completely missed. However I have a few concerns
>>> on what approaches I could take.
>>>
>>> Attempting to use a guard in this situation would require the guard to be moved to the top of the function,
>>> resuming the system much early in the stack.
>>>
>>> As far as I understand this is due to the guard not being able to succeed any gotos as the "lock" variable
>>> in the guard must be initialized before any subsequent goto.
>>>
>>> For example:
>>>
>>> goto error;
>>> PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
> Oh, that is a very good point.
>
>>> Is invalid as "lock" is initialized after the goto.
>>> Trying to initialize lock beforehand doesn't work as it would count as a redefinition of lock instead.
>>>
>>> Would that still be preferred over creating a new label and trying something like this instead?
> Yes, as far as I can see the PM reference just needs to be moved before the first goto, right?
>
> When I now check the code, the first goto in function amdgpu_gem_va_ioctl() is that here:
>
>          /* Add input syncobj fences (if any) for synchronization. */
>          r = amdgpu_gem_add_input_fence(filp,
>                                         args->input_fence_syncobj_handles,
>                                         args->num_syncobj_handles);
>          if (r)
>                  goto error_put_gobj;
>
>          drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT |
>                        DRM_EXEC_IGNORE_DUPLICATES, 0);
>
>
> We can't grab the PM reference after drm_exec_init() anyway because that could deadlock.
>
> So that should still work as far as I can see, or do I miss anything?
>
> Regards,
> Christian.
Nope, you got everything. Just making sure if this approach was the one 
that should be pursued.

I didn't notice the drm_exec in this case either.

Thanks for the help,
Shahyan
>>> replace
>>>
>>> +    r = pm_runtime_resume_and_get(adev->dev);
>>> +    if (r < 0)
>>> +        return r;
>>>
>>> with
>>>
>>> +    r = pm_runtime_resume_and_get(adev->dev);
>>> +    if (r < 0)
>>> +        goto newlabel;
>>>
>>>      }
>>>
>>> ...
>>>
>>> + newlabel:
>>> +    pm_runtime_put_autosuspend(adev->dev);
>>> error:
>>>      dma_fence_chain_free(timeline_chain);
>>>      if (timeline_syncobj)
>>>          drm_syncobj_put(timeline_syncobj);
>>>      drm_exec_fini(&exec);
>>> error_put_gobj:
>>>      drm_gem_object_put(gobj);
>>>      return r;
>>> }
>>>
>>> Another approach I could try is wrap the entirePM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); and the subsequent switch cases before the error handling into its own scope with {} However, I feel this method is a lot more hacky then necessary. int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) { ... + { + PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); + + r = PM_RUNTIME_ACQUIRE_ERR(&lock); + if (r) + goto error; switch (args->operation) { case AMDGPU_VA_OP_MAP: ... case AMDGPU_VA_OP_UNMAP: .. case AMDGPU_VA_OP_CLEAR: ... case AMDGPU_VA_OP_REPLACE: ... default: ... } if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) && (!adev->debug_vm || timeline_syncobj)) { ... } + } error: ... error_put_gobj: ... } If you have any ideas on what approach I should/could take it would be very much appreciated. Thanks, Shahyan
>>>
>> I'm not entirely sure what happened here...
>>
>> But I meant to say wrapping the guard and the subsequent case handling into its own scope.
>>
>> For example:
>>
>> int amdgpu_gem_va_ioctl() {
>> ...
>>      goto error;
>>
>> +    {
>> +        PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
>> +        r = PM_RUNTIME_ACQUIRE_ERR(&lock);
>> +        if (r)
>> +            goto error;
>>
>> ...
>> +    }
>> error:
>> ...
>> error_put_gobj:
>> ...
>> }
>>
>> Thanks,
>> Shahyan
>>
>>>>> +
>>>>>    error:
>>>>>        dma_fence_chain_free(timeline_chain);
>>>>>        if (timeline_syncobj)