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 <[email protected]>
> ---
> 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;
