[PATCH v3 0/2] drm/amdgpu: rework amdgpu open() path to function in sleep state

[PATCH v3 0/2] drm/amdgpu: rework amdgpu open() path to function in sleep state

[Shahyan Soltani]

This patch series reworks the amdgpu open/close, vm, cs, and validation
functionality to allow for access of the GPU's CPU sided bookkeeping
data without waking up the GPU from idle.

The overhaul will allow a few tools to function without waking up the
GPU, which can improve power efficiency within relevant domains, and can
hopefully be expanded on later to adapt for useful utilities.

v2:

- Force a vm->generation mismatch by adding 1 to the result of
amdgpu_vm_generation.

- Separate validation logic from patch "drm/amdgpu: defer GPU resume from
open/close to first use" into a new patch.

- Change validation logic to not depend on flags to operate. Instead
relying on a vm->generation mismatch.

- Clear vm pts in the kernel.evicted loop instead.

- Remove the root_cleared flag.

- Let the vm domain be initialized to VRAM in amdgpu_vm_pt_create if the
vm->root.bo is not null.

- Clear the root page table on vm_init if pm_ref was true.

v3:

- Merged: commit fbbcdad4c7c8 ("drm/amdgpu: clear vm pts upon a
  vm->generation mismatch")

- Remove pm_runtime_get_sync() and pm_runtime_put_autosuspend() from
  open/postclose kms.

- Remove the powered_init flag and parameter to vm_init.

- Remove amdgpu_vm_generation() result + 1 patch to instead depend on
  setting vm->generation to ~0ULL to force the mismatch.

- Initialize the root PD's domain to 0 (system memory) instead of GTT.

- Add validation handling in accordance with amdgpu_ualink.c vm
  initialization.

Concerns:

In the first patch, amdgpu_vm_init() reserved a fence slot for
amdgpu_vm_pt_clear() prior to this patch set:

	r = dma_resv_reserve_fences(root_bo->tbo.base.resv, 1);
	if (r)
		goto error_free_root;
-	r = amdgpu_vm_pt_clear(adev, vm, root, false);

Due to my limited knowledge on how fences function and when they are
required, I wasn't sure whether I had to replicate this in ualink and
possibly other locations that require amdgpu_vm_validate(). Such as in
amdgpu_amdkfd_gpuvm.c. Since amdgpu_vm_pt_clear is now deferred to
validate.

As a result I left it untouched within those domains and used
amdgpu_ualink_reserve_npa_vm_and_bos() instead of amdgpu_bo_reserve()
for ualink, since the former is native to ualink and reserves both the
bo and fence slots.

This can be easily changed into amdgpu_bo_reserve() in v4 of the patch
if required.

In the second patch, amdgpu_driver_postclose_kms calls functions
amdgpu_uvd_free_handles and amdgpu_vce_free_handles. Both of which
require hardware access. I attempted to test this with a Radeon VII
card, which to my knowledge does utilize these multimedia
specifications. However, I was unable to get the gpu to enter BACO/BOCO
mode and wasn't fully able to test the path.

I'm not sure if it was an exception to my card or BACO/BOCO isn't
supported on those older GFX IPs.

Shahyan Soltani (2):
  drm/amdgpu: defer amdgpu_vm_pt_clear() to validation
  drm/amdgpu: remove pm_runtime from open/close kms paths

 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 18 +++----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c | 31 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  6 +----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c  | 13 +++++++--
 5 files changed, 47 insertions(+), 22 deletions(-)

-- 
2.56.0

[PATCH v3 1/2] drm/amdgpu: defer amdgpu_vm_pt_clear() to validation

[Shahyan Soltani]

Currently amdgpu_vm_pt_clear() is called at open time in
amdgpu_vm_init(). It submits an SDMA job, which needs the GPU to be
awake and adds complications when it comes to preventing the GPU from
resuming when direct hardware access isn't required. Instead,
amdgpu_vm_pt_clear() can be moved out of amdgpu_vm_init() to run later
on when it's required, such as when amdgpu_vm_validate() is called.

Introduce delayed virtual memory hardware initialization by changing the
vm->generation assignment on amdgpu_vm_init() to ~0ULL. By setting
vm->generation to this value, it will force a mismatch to occur on init,
processing the amdgpu_vm_validate() logic, and clearing the root PT much
later on in the stack.

Set the initial domain of vm->root.bo to 0, this will allow all contents
of the root PT to exist in system memory without requiring the GPU to be
active. The vm->root.bo is then put on the evicted list, which will then
migrate the PT to its preferred/allowed domain at the best possible
time (usually through amdgpu_vm_validate() on the CS path).

Edit amdgpu_ualink_manager_start() to no longer depend on
amdgpu_vm_pt_clear() within amdgpu_vm_init(). Instead, the root bo will
be reserved directly after initializing the npa vm and
amdgpu_vm_validate() will be called within the reservation, using the
logic of the new amdgpu_ualink_bo_validate() function to process the
validation.

Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c | 31 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  6 +----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c  | 13 +++++++--
 4 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
index 6cd3927a7e82..1e8f7fe8e1fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
@@ -27,6 +27,7 @@
 #include <linux/random.h>
 #include "amdgpu.h"
 #include "amdgpu_sdma.h"
+#include "amdgpu_vm.h"
 #include "amdgpu_ualink.h"
 #include "amdgpu_xgmi.h"
 #include "amdgpu_dma_buf.h"
@@ -4165,14 +4166,44 @@ int amdgpu_gem_ualink_handle_ioctl(struct drm_device *dev, void *data,
 	return r;
 }
 
+int amdgpu_ualink_bo_validate(void *param, struct amdgpu_bo *bo)
+{
+	struct ttm_operation_ctx ctx = { false, false };
+	int r;
+
+	if (bo->tbo.pin_count)
+		return 0;
+
+	amdgpu_bo_placement_from_domain(bo, bo->preferred_domains);
+
+	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
+	return r;
+}
+
 int amdgpu_ualink_manager_start(struct amdgpu_device *adev)
 {
+	struct drm_exec exec;
 	int i, r;
 
 	r = amdgpu_vm_init(adev, &adev->ualink.npa_vm, 0);
 	if (r)
 		goto out;
 
+	r = amdgpu_ualink_reserve_npa_vm_and_bos(adev, &adev->ualink.npa_vm.root.bo,
+						 0, &exec, false);
+	if (unlikely(r)) {
+		dev_err(adev->dev, "Failed to reserve VM in ualink_manager_start\n");
+		goto uninit_vm;
+	}
+
+	/* Ensure the root PT is migrated to the correct domain */
+	r = amdgpu_vm_validate(adev, &adev->ualink.npa_vm, NULL, amdgpu_ualink_bo_validate, NULL);
+	amdgpu_ualink_unreserve_npa_vm_and_bos(adev, &exec);
+	if (r) {
+		drm_err(adev_to_drm(adev), "amdgpu_vm_validate() failed.\n");
+		goto uninit_vm;
+	}
+
 	/* For pinning page tables and using CPU for page table updates. */
 	r = amdgpu_vm_make_npa(adev, &adev->ualink.npa_vm);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
index 85c9a9634a4b..399ff4da0444 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
@@ -405,6 +405,7 @@ int amdgpu_ualink_resume_handler(struct amdgpu_device *adev);
 int amdgpu_ualink_sysfs_init(struct amdgpu_device *adev);
 void amdgpu_ualink_sysfs_fini(struct amdgpu_device *adev);
 void amdgpu_ualink_xcp_sysfs_update(struct amdgpu_xcp *xcp);
+int amdgpu_ualink_bo_validate(void *param, struct amdgpu_bo *bo);
 int amdgpu_ualink_manager_start(struct amdgpu_device *adev);
 void amdgpu_ualink_manager_stop(struct amdgpu_device *adev);
 int amdgpu_ualink_export_handle(struct drm_device *dev, struct drm_file *filp,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 000c982816a7..088a4e5a1aa2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2660,6 +2660,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	vm->is_compute_context = false;
 	vm->need_tlb_fence = amdgpu_userq_enabled(&adev->ddev);
+	vm->generation = ~0ULL;
 
 	vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
 				    AMDGPU_VM_USE_CPU_FOR_GFX);
@@ -2678,7 +2679,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;
@@ -2701,10 +2701,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_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index c73eb416c0e2..2b9f8aafb685 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -449,6 +449,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 {
 	struct amdgpu_bo_param bp;
 	unsigned int num_entries;
+	u32 set_domain;
 	int r;
 
 	memset(&bp, 0, sizeof(bp));
@@ -457,9 +458,11 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
 
 	if (!adev->gmc.is_app_apu)
-		bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
+		set_domain = AMDGPU_GEM_DOMAIN_VRAM;
 	else
-		bp.domain = AMDGPU_GEM_DOMAIN_GTT;
+		set_domain = AMDGPU_GEM_DOMAIN_GTT;
+
+	bp.domain = (!vm->root.bo) ? 0 : set_domain;
 
 	bp.domain = amdgpu_bo_get_preferred_domain(adev, bp.domain);
 	bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
@@ -486,6 +489,12 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		return r;
 
+	if (!vm->root.bo) {
+		(*vmbo)->bo.preferred_domains = set_domain;
+		(*vmbo)->bo.allowed_domains = (set_domain == AMDGPU_GEM_DOMAIN_VRAM) ?
+			AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT : set_domain;
+	}
+
 	/* Assumes that reservation is shared with the VM root and that the
 	 * reservation is locked
 	 */
-- 
2.56.0

Re: [PATCH v3 1/2] drm/amdgpu: defer amdgpu_vm_pt_clear() to validation

[Christian König]

On 10/29/26 05:09, Shahyan Soltani wrote:
> Currently amdgpu_vm_pt_clear() is called at open time in
> amdgpu_vm_init(). It submits an SDMA job, which needs the GPU to be
> awake and adds complications when it comes to preventing the GPU from
> resuming when direct hardware access isn't required. Instead,
> amdgpu_vm_pt_clear() can be moved out of amdgpu_vm_init() to run later
> on when it's required, such as when amdgpu_vm_validate() is called.
> 
> Introduce delayed virtual memory hardware initialization by changing the
> vm->generation assignment on amdgpu_vm_init() to ~0ULL. By setting
> vm->generation to this value, it will force a mismatch to occur on init,
> processing the amdgpu_vm_validate() logic, and clearing the root PT much
> later on in the stack.
> 
> Set the initial domain of vm->root.bo to 0, this will allow all contents
> of the root PT to exist in system memory without requiring the GPU to be
> active. The vm->root.bo is then put on the evicted list, which will then
> migrate the PT to its preferred/allowed domain at the best possible
> time (usually through amdgpu_vm_validate() on the CS path).
> 
> Edit amdgpu_ualink_manager_start() to no longer depend on
> amdgpu_vm_pt_clear() within amdgpu_vm_init(). Instead, the root bo will
> be reserved directly after initializing the npa vm and
> amdgpu_vm_validate() will be called within the reservation, using the
> logic of the new amdgpu_ualink_bo_validate() function to process the
> validation.
> 
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c | 31 ++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  6 +----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c  | 13 +++++++--
>  4 files changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> index 6cd3927a7e82..1e8f7fe8e1fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> @@ -27,6 +27,7 @@
>  #include <linux/random.h>
>  #include "amdgpu.h"
>  #include "amdgpu_sdma.h"
> +#include "amdgpu_vm.h"
>  #include "amdgpu_ualink.h"
>  #include "amdgpu_xgmi.h"
>  #include "amdgpu_dma_buf.h"
> @@ -4165,14 +4166,44 @@ int amdgpu_gem_ualink_handle_ioctl(struct drm_device *dev, void *data,
>  	return r;
>  }
>  
> +int amdgpu_ualink_bo_validate(void *param, struct amdgpu_bo *bo)
> +{
> +	struct ttm_operation_ctx ctx = { false, false };
> +	int r;
> +
> +	if (bo->tbo.pin_count)
> +		return 0;
> +
> +	amdgpu_bo_placement_from_domain(bo, bo->preferred_domains);
> +
> +	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> +	return r;
> +}
> +
>  int amdgpu_ualink_manager_start(struct amdgpu_device *adev)
>  {
> +	struct drm_exec exec;
>  	int i, r;
>  
>  	r = amdgpu_vm_init(adev, &adev->ualink.npa_vm, 0);
>  	if (r)
>  		goto out;
>  
> +	r = amdgpu_ualink_reserve_npa_vm_and_bos(adev, &adev->ualink.npa_vm.root.bo,
> +						 0, &exec, false);
> +	if (unlikely(r)) {
> +		dev_err(adev->dev, "Failed to reserve VM in ualink_manager_start\n");
> +		goto uninit_vm;
> +	}
> +
> +	/* Ensure the root PT is migrated to the correct domain */
> +	r = amdgpu_vm_validate(adev, &adev->ualink.npa_vm, NULL, amdgpu_ualink_bo_validate, NULL);
> +	amdgpu_ualink_unreserve_npa_vm_and_bos(adev, &exec);
> +	if (r) {
> +		drm_err(adev_to_drm(adev), "amdgpu_vm_validate() failed.\n");
> +		goto uninit_vm;
> +	}
> +


>  	/* For pinning page tables and using CPU for page table updates. */
>  	r = amdgpu_vm_make_npa(adev, &adev->ualink.npa_vm);

That function call needs to validate/pin the root PD anyway, adding clearing to it should be much more trivial than open code it here.

Writing this the NPA mode needs to use a different value to clear the root PD. So that the root PD isn't cleared again after setting vm->is_npa is most likely a bug.

Please separate that change out of this patch and make sure to CC Felix and whoever worked on the NPA patches for the review.

Regards,
Christian.

>  	if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
> index 85c9a9634a4b..399ff4da0444 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
> @@ -405,6 +405,7 @@ int amdgpu_ualink_resume_handler(struct amdgpu_device *adev);
>  int amdgpu_ualink_sysfs_init(struct amdgpu_device *adev);
>  void amdgpu_ualink_sysfs_fini(struct amdgpu_device *adev);
>  void amdgpu_ualink_xcp_sysfs_update(struct amdgpu_xcp *xcp);
> +int amdgpu_ualink_bo_validate(void *param, struct amdgpu_bo *bo);
>  int amdgpu_ualink_manager_start(struct amdgpu_device *adev);
>  void amdgpu_ualink_manager_stop(struct amdgpu_device *adev);
>  int amdgpu_ualink_export_handle(struct drm_device *dev, struct drm_file *filp,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 000c982816a7..088a4e5a1aa2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2660,6 +2660,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  
>  	vm->is_compute_context = false;
>  	vm->need_tlb_fence = amdgpu_userq_enabled(&adev->ddev);
> +	vm->generation = ~0ULL;
>  
>  	vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
>  				    AMDGPU_VM_USE_CPU_FOR_GFX);
> @@ -2678,7 +2679,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;
> @@ -2701,10 +2701,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_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index c73eb416c0e2..2b9f8aafb685 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -449,6 +449,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  {
>  	struct amdgpu_bo_param bp;
>  	unsigned int num_entries;
> +	u32 set_domain;
>  	int r;
>  
>  	memset(&bp, 0, sizeof(bp));
> @@ -457,9 +458,11 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
>  
>  	if (!adev->gmc.is_app_apu)
> -		bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
> +		set_domain = AMDGPU_GEM_DOMAIN_VRAM;
>  	else
> -		bp.domain = AMDGPU_GEM_DOMAIN_GTT;
> +		set_domain = AMDGPU_GEM_DOMAIN_GTT;
> +
> +	bp.domain = (!vm->root.bo) ? 0 : set_domain;
>  
>  	bp.domain = amdgpu_bo_get_preferred_domain(adev, bp.domain);
>  	bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
> @@ -486,6 +489,12 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	if (r)
>  		return r;
>  
> +	if (!vm->root.bo) {
> +		(*vmbo)->bo.preferred_domains = set_domain;
> +		(*vmbo)->bo.allowed_domains = (set_domain == AMDGPU_GEM_DOMAIN_VRAM) ?
> +			AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT : set_domain;
> +	}
> +
>  	/* Assumes that reservation is shared with the VM root and that the
>  	 * reservation is locked
>  	 */

[PATCH v3 2/2] drm/amdgpu: remove pm_runtime from open/close kms paths

[Shahyan Soltani]

Every open system call for an amdgpu device holds a runtime reference
and resumes the device from a BACO/BOCO sleep state even if direct
hardware access is not required.

Drop pm_runtime_get_sync() and pm_runtime_put_autosuspend() from
open/close KMS to prevent unnecessary resumes.

Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 14a72a406d37..05406c9ff370 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1506,7 +1506,6 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 	/* Ensure IB tests are run on ring */
 	flush_delayed_work(&adev->delayed_init_work);
 
-
 	if (amdgpu_ras_intr_triggered()) {
 		DRM_ERROR("RAS Intr triggered, device disabled!!");
 		return -EHWPOISON;
@@ -1514,14 +1513,10 @@ 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;
-
 	fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
 	if (unlikely(!fpriv)) {
 		r = -ENOMEM;
-		goto out_suspend;
+		goto out;
 	}
 
 	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
@@ -1583,7 +1578,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 	amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
 
 	file_priv->driver_priv = fpriv;
-	goto out_suspend;
+	goto out;
 
 error_vm:
 	if (pasid) {
@@ -1596,10 +1591,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 error_pasid:
 	kfree(fpriv);
 
-out_suspend:
-pm_put:
-	pm_runtime_put_autosuspend(dev->dev);
-
+out:
 	return r;
 }
 
@@ -1624,8 +1616,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
 	if (!fpriv)
 		return;
 
-	pm_runtime_get_sync(dev->dev);
-
 	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_UVD) != NULL)
 		amdgpu_uvd_free_handles(adev, file_priv);
 	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_VCE) != NULL)
@@ -1662,8 +1652,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
 
 	kfree(fpriv);
 	file_priv->driver_priv = NULL;
-
-	pm_runtime_put_autosuspend(dev->dev);
 }
 
 
-- 
2.56.0

Re: [PATCH v3 2/2] drm/amdgpu: remove pm_runtime from open/close kms paths

[Christian König]

On 10/29/26 05:09, Shahyan Soltani wrote:
> Every open system call for an amdgpu device holds a runtime reference
> and resumes the device from a BACO/BOCO sleep state even if direct
> hardware access is not required.
> 
> Drop pm_runtime_get_sync() and pm_runtime_put_autosuspend() from
> open/close KMS to prevent unnecessary resumes.
> 
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>

Please triple check that we haven't missed anything and the functions now get/put the PM reference as necessary.

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

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 14a72a406d37..05406c9ff370 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1506,7 +1506,6 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	/* Ensure IB tests are run on ring */
>  	flush_delayed_work(&adev->delayed_init_work);
>  
> -
>  	if (amdgpu_ras_intr_triggered()) {
>  		DRM_ERROR("RAS Intr triggered, device disabled!!");
>  		return -EHWPOISON;
> @@ -1514,14 +1513,10 @@ 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;
> -
>  	fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
>  	if (unlikely(!fpriv)) {
>  		r = -ENOMEM;
> -		goto out_suspend;
> +		goto out;
>  	}
>  
>  	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
> @@ -1583,7 +1578,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
>  
>  	file_priv->driver_priv = fpriv;
> -	goto out_suspend;
> +	goto out;
>  
>  error_vm:
>  	if (pasid) {
> @@ -1596,10 +1591,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  error_pasid:
>  	kfree(fpriv);
>  
> -out_suspend:
> -pm_put:
> -	pm_runtime_put_autosuspend(dev->dev);
> -
> +out:
>  	return r;
>  }
>  
> @@ -1624,8 +1616,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  	if (!fpriv)
>  		return;
>  
> -	pm_runtime_get_sync(dev->dev);
> -
>  	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_UVD) != NULL)
>  		amdgpu_uvd_free_handles(adev, file_priv);
>  	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_VCE) != NULL)
> @@ -1662,8 +1652,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  
>  	kfree(fpriv);
>  	file_priv->driver_priv = NULL;
> -
> -	pm_runtime_put_autosuspend(dev->dev);
>  }
>  
>  

Re: [PATCH v3 2/2] drm/amdgpu: remove pm_runtime from open/close kms paths

[Christian König]

On 10/29/26 05:09, Shahyan Soltani wrote:
> Every open system call for an amdgpu device holds a runtime reference
> and resumes the device from a BACO/BOCO sleep state even if direct
> hardware access is not required.
> 
> Drop pm_runtime_get_sync() and pm_runtime_put_autosuspend() from
> open/close KMS to prevent unnecessary resumes.
> 
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>

Please triple check that we haven't missed anything and the functions now get/put the PM reference as necessary.

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

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 14a72a406d37..05406c9ff370 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1506,7 +1506,6 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	/* Ensure IB tests are run on ring */
>  	flush_delayed_work(&adev->delayed_init_work);
>  
> -
>  	if (amdgpu_ras_intr_triggered()) {
>  		DRM_ERROR("RAS Intr triggered, device disabled!!");
>  		return -EHWPOISON;
> @@ -1514,14 +1513,10 @@ 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;
> -
>  	fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
>  	if (unlikely(!fpriv)) {
>  		r = -ENOMEM;
> -		goto out_suspend;
> +		goto out;
>  	}
>  
>  	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
> @@ -1583,7 +1578,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
>  
>  	file_priv->driver_priv = fpriv;
> -	goto out_suspend;
> +	goto out;
>  
>  error_vm:
>  	if (pasid) {
> @@ -1596,10 +1591,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  error_pasid:
>  	kfree(fpriv);
>  
> -out_suspend:
> -pm_put:
> -	pm_runtime_put_autosuspend(dev->dev);
> -
> +out:
>  	return r;
>  }
>  
> @@ -1624,8 +1616,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  	if (!fpriv)
>  		return;
>  
> -	pm_runtime_get_sync(dev->dev);
> -
>  	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_UVD) != NULL)
>  		amdgpu_uvd_free_handles(adev, file_priv);
>  	if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_VCE) != NULL)
> @@ -1662,8 +1652,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  
>  	kfree(fpriv);
>  	file_priv->driver_priv = NULL;
> -
> -	pm_runtime_put_autosuspend(dev->dev);
>  }
>  
>