[Patch v1 5/5] drm/amdkfd: refactor runtime pm for baco
Felix Kuehling
felix.kuehling at amd.com
Wed Jan 29 22:52:18 UTC 2020
HI Rajneesh,
See comments inline ...
And a general question: Why do you need to set the autosuspend_delay in
so many places? Amdgpu only has a single call to this function during
initialization.
On 2020-01-27 20:29, Rajneesh Bhardwaj wrote:
> So far the kfd driver implemented same routines for runtime and system
> wide suspend and resume (s2idle or mem). During system wide suspend the
> kfd aquires an atomic lock that prevents any more user processes to
> create queues and interact with kfd driver and amd gpu. This mechanism
> created problem when amdgpu device is runtime suspended with BACO
> enabled. Any application that relies on kfd driver fails to load because
> the driver reports a locked kfd device since gpu is runtime suspended.
>
> However, in an ideal case, when gpu is runtime suspended the kfd driver
> should be able to:
>
> - auto resume amdgpu driver whenever a client requests compute service
> - prevent runtime suspend for amdgpu while kfd is in use
>
> This change refactors the amdgpu and amdkfd drivers to support BACO and
> runtime runtime power management.
>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 12 ++++-----
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 8 +++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +--
> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 31 +++++++++++++---------
> drivers/gpu/drm/amd/amdkfd/kfd_iommu.c | 5 +++-
> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 19 ++++++++++---
> 6 files changed, 51 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 8609287620ea..314c4a2a0354 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -178,18 +178,18 @@ void amdgpu_amdkfd_interrupt(struct amdgpu_device *adev,
> kgd2kfd_interrupt(adev->kfd.dev, ih_ring_entry);
> }
>
> -void amdgpu_amdkfd_suspend(struct amdgpu_device *adev)
> +void amdgpu_amdkfd_suspend(struct amdgpu_device *adev, bool run_pm)
> {
> if (adev->kfd.dev)
> - kgd2kfd_suspend(adev->kfd.dev);
> + kgd2kfd_suspend(adev->kfd.dev, run_pm);
> }
>
> -int amdgpu_amdkfd_resume(struct amdgpu_device *adev)
> +int amdgpu_amdkfd_resume(struct amdgpu_device *adev, bool run_pm)
> {
> int r = 0;
>
> if (adev->kfd.dev)
> - r = kgd2kfd_resume(adev->kfd.dev);
> + r = kgd2kfd_resume(adev->kfd.dev, run_pm);
>
> return r;
> }
> @@ -713,11 +713,11 @@ void kgd2kfd_exit(void)
> {
> }
>
> -void kgd2kfd_suspend(struct kfd_dev *kfd)
> +void kgd2kfd_suspend(struct kfd_dev *kfd, bool run_pm)
> {
> }
>
> -int kgd2kfd_resume(struct kfd_dev *kfd)
> +int kgd2kfd_resume(struct kfd_dev *kfd, bool run_pm)
> {
> return 0;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 0fa898d30207..3dce4a51e522 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -123,8 +123,8 @@ struct amdkfd_process_info {
> int amdgpu_amdkfd_init(void);
> void amdgpu_amdkfd_fini(void);
>
> -void amdgpu_amdkfd_suspend(struct amdgpu_device *adev);
> -int amdgpu_amdkfd_resume(struct amdgpu_device *adev);
> +void amdgpu_amdkfd_suspend(struct amdgpu_device *adev, bool run_pm);
> +int amdgpu_amdkfd_resume(struct amdgpu_device *adev, bool run_pm);
> void amdgpu_amdkfd_interrupt(struct amdgpu_device *adev,
> const void *ih_ring_entry);
> void amdgpu_amdkfd_device_probe(struct amdgpu_device *adev);
> @@ -250,8 +250,8 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
> struct drm_device *ddev,
> const struct kgd2kfd_shared_resources *gpu_resources);
> void kgd2kfd_device_exit(struct kfd_dev *kfd);
> -void kgd2kfd_suspend(struct kfd_dev *kfd);
> -int kgd2kfd_resume(struct kfd_dev *kfd);
> +void kgd2kfd_suspend(struct kfd_dev *kfd, bool run_pm);
> +int kgd2kfd_resume(struct kfd_dev *kfd, bool run_pm);
> int kgd2kfd_pre_reset(struct kfd_dev *kfd);
> int kgd2kfd_post_reset(struct kfd_dev *kfd);
> void kgd2kfd_interrupt(struct kfd_dev *kfd, const void *ih_ring_entry);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d9e5eac182d3..787d49e9f4de 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3314,7 +3314,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
> }
> }
>
> - amdgpu_amdkfd_suspend(adev);
> + amdgpu_amdkfd_suspend(adev, fbcon);
The logic seems inverted here. There are four different pmops callbacks
that use different values for this parameter:
* amdgpu_pmops_suspend: true
* amdgpu_pmops_freeze: true
* amdgpu_pmops_poweroff: true
* amdgpu_pmops_runtime_suspend: false
It looks like runtime_suspend uses false, so you should pass the run_pm
parameter as !fbcon.
>
> amdgpu_ras_suspend(adev);
>
> @@ -3398,7 +3398,7 @@ int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
> }
> }
> }
> - r = amdgpu_amdkfd_resume(adev);
> + r = amdgpu_amdkfd_resume(adev, fbcon);
> if (r)
> return r;
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 2a9e40131735..e9f00c3a067a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -23,6 +23,7 @@
> #include <linux/bsearch.h>
> #include <linux/pci.h>
> #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
> #include "kfd_priv.h"
> #include "kfd_device_queue_manager.h"
> #include "kfd_pm4_headers_vi.h"
> @@ -710,7 +711,7 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
> void kgd2kfd_device_exit(struct kfd_dev *kfd)
> {
> if (kfd->init_complete) {
> - kgd2kfd_suspend(kfd);
> + kgd2kfd_suspend(kfd, true);
> device_queue_manager_uninit(kfd->dqm);
> kfd_interrupt_exit(kfd);
> kfd_topology_remove_device(kfd);
> @@ -731,7 +732,7 @@ int kgd2kfd_pre_reset(struct kfd_dev *kfd)
>
> kfd->dqm->ops.pre_reset(kfd->dqm);
>
> - kgd2kfd_suspend(kfd);
> + kgd2kfd_suspend(kfd, true);
This should use false. This is not runtime PM.
>
> kfd_signal_reset_event(kfd);
> return 0;
> @@ -765,21 +766,24 @@ bool kfd_is_locked(void)
> return (atomic_read(&kfd_locked) > 0);
> }
>
> -void kgd2kfd_suspend(struct kfd_dev *kfd)
> +void kgd2kfd_suspend(struct kfd_dev *kfd, bool run_pm)
> {
> if (!kfd->init_complete)
> return;
>
> - /* For first KFD device suspend all the KFD processes */
> - if (atomic_inc_return(&kfd_locked) == 1)
> - kfd_suspend_all_processes();
> + /* for runtime suspend, skip locking kfd */
> + if (run_pm) {
This should be done for the non-runtime PM case: if (!run_pm).
> + /* For first KFD device suspend all the KFD processes */
> + if (atomic_inc_return(&kfd_locked) == 1)
> + kfd_suspend_all_processes();
> + }
>
> + pm_runtime_set_autosuspend_delay(kfd->ddev->dev, 5000);
> kfd->dqm->ops.stop(kfd->dqm);
> -
> kfd_iommu_suspend(kfd);
> }
>
> -int kgd2kfd_resume(struct kfd_dev *kfd)
> +int kgd2kfd_resume(struct kfd_dev *kfd, bool run_pm)
> {
> int ret, count;
>
> @@ -790,10 +794,13 @@ int kgd2kfd_resume(struct kfd_dev *kfd)
> if (ret)
> return ret;
>
> - count = atomic_dec_return(&kfd_locked);
> - WARN_ONCE(count < 0, "KFD suspend / resume ref. error");
> - if (count == 0)
> - ret = kfd_resume_all_processes();
> + /* for runtime resume, skip unlocking kfd */
> + if (run_pm) {
Same as above.
> + count = atomic_dec_return(&kfd_locked);
> + WARN_ONCE(count < 0, "KFD suspend / resume ref. error");
> + if (count == 0)
> + ret = kfd_resume_all_processes();
> + }
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
> index 8d871514671e..6301d77ed3d6 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
> @@ -25,6 +25,7 @@
> #include <linux/slab.h>
> #include <linux/pci.h>
> #include <linux/amd-iommu.h>
> +#include <linux/pm_runtime.h>
> #include "kfd_priv.h"
> #include "kfd_dbgmgr.h"
> #include "kfd_topology.h"
> @@ -134,8 +135,10 @@ void kfd_iommu_unbind_process(struct kfd_process *p)
> struct kfd_process_device *pdd;
>
> list_for_each_entry(pdd, &p->per_device_data, per_device_list)
> - if (pdd->bound == PDD_BOUND)
> + if (pdd->bound == PDD_BOUND) {
> amd_iommu_unbind_pasid(pdd->dev->pdev, p->pasid);
> + pm_runtime_set_autosuspend_delay(pdd->dev->ddev->dev, 5000);
I don't think this is the right place. This code should only run on APUs
with IOMMUv2 enabled. Probably missing a check at the start of the
function. On kernels with IOMMU disabled, this source file is not
compiled at all.
I think this code should go into kfd_process_destroy_pdds.
> + }
> }
>
> /* Callback for process shutdown invoked by the IOMMU driver */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 25b90f70aecd..d19d5e97405c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -31,6 +31,7 @@
> #include <linux/compat.h>
> #include <linux/mman.h>
> #include <linux/file.h>
> +#include <linux/pm_runtime.h>
> #include "amdgpu_amdkfd.h"
> #include "amdgpu.h"
>
> @@ -843,15 +844,27 @@ struct kfd_process_device *kfd_bind_process_to_device(struct kfd_dev *dev,
> return ERR_PTR(-ENOMEM);
> }
>
> + err = pm_runtime_get_sync(dev->ddev->dev);
> + pm_runtime_set_autosuspend_delay(dev->ddev->dev, 60000);
Why are you using a different hard-coded delay (60s?) here?
Regards,
Felix
> + if (err < 0)
> + return ERR_PTR(err);
> +
> err = kfd_iommu_bind_process_to_device(pdd);
> if (err)
> - return ERR_PTR(err);
> + goto out;
>
> err = kfd_process_device_init_vm(pdd, NULL);
> if (err)
> - return ERR_PTR(err);
> + goto out;
>
> - return pdd;
> +out:
> + pm_runtime_mark_last_busy(dev->ddev->dev);
> + pm_runtime_put_autosuspend(dev->ddev->dev);
> +
> + if (!err)
> + return pdd;
> + else
> + return ERR_PTR(err);
> }
>
> struct kfd_process_device *kfd_get_first_process_device_data(
More information about the amd-gfx
mailing list