答复: [PATCH v2] drm/amd/amdgpu: S4 issue for amdgpu

[Qu, Jim]

Hi Alex:

According to my understanding, in our driver, amdgpu_device_suspend/resume. the paremeter 'suspend' or 'resume' mean the apdpter is set to D3, and we need power on it at resume. In S4 , the function freeze() , thraw() , poweroff() and restore() is special for hibernation,  S3 process could not call these functions.  so far, according to  my experiment, we must reset asic if adapter is not power down in S4, otherwise, it will be GFX(compute) , SDMA ring test failure on VI(not sure it will be on SI), it seems that aisc reset is not must on CI.

I do not review our runtime PM code, but, based on currently code ,  I can be sure that asic must be reset if it is suspended without powerdown.

BTW, driver dpm does not work when resume back after hibernation on CI.

Thanks
JimQu

________________________________________
发件人: Deucher, Alexander
发送时间: 2016年9月7日 19:15:22
收件人: Qu, Jim; amd-gfx at lists.freedesktop.org
抄送: Qu, Jim
主题: RE: [PATCH v2] drm/amd/amdgpu: S4 issue for amdgpu

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
> Of jimqu
> Sent: Saturday, September 03, 2016 2:57 AM
> To: amd-gfx at lists.freedesktop.org
> Cc: Qu, Jim
> Subject: [PATCH v2] drm/amd/amdgpu: S4 issue for amdgpu
>
> reset the asic if adapter is not powerdown when doing freeze()
> thaw() and restore(), in order to get a valid state of adapter.
>
> Change-Id: Ibb03ab3c2ce6ef3c83affbe767308dc22474f563
> Signed-off-by: JimQu <Jim.Qu at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 19 +++++++++++++-----
> -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 18 ++++++++++++++++--
>  2 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6510514..f17fc6d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1937,6 +1937,10 @@ int amdgpu_device_suspend(struct drm_device
> *dev, bool suspend, bool fbcon)
>               /* Shut down the device */
>               pci_disable_device(dev->pdev);
>               pci_set_power_state(dev->pdev, PCI_D3hot);
> +     } else {
> +             r = amdgpu_asic_reset(adev);
> +             if (r)
> +                     DRM_ERROR("amdgpu asic reset failed\n");

Do we want to guard this with a separate parameter?   Are there use cases where we may not want D3 or GPU reset?  Runtime pm for PX comes to mind.  That said, it probably won't hurt in that case either.

Alex

>       }
>
>       if (fbcon) {
> @@ -1967,22 +1971,25 @@ int amdgpu_device_resume(struct drm_device
> *dev, bool resume, bool fbcon)
>           dev->switch_power_state ==
> DRM_SWITCH_POWER_DYNAMIC_OFF)
>               return 0;
>
> -     if (fbcon) {
> +     if (fbcon)
>               console_lock();
> -     }
> +
>       if (resume) {
>               pci_set_power_state(dev->pdev, PCI_D0);
>               pci_restore_state(dev->pdev);
> -             if (pci_enable_device(dev->pdev)) {
> +             if (r = pci_enable_device(dev->pdev)) {
>                       if (fbcon)
>                               console_unlock();
> -                     return -1;
> +                     return r;
>               }
>       }
>
>       /* post card */
> -     if (!amdgpu_card_posted(adev))
> -             amdgpu_atom_asic_init(adev->mode_info.atom_context);
> +     if (!amdgpu_card_posted(adev) || !resume) {
> +             r = amdgpu_atom_asic_init(adev-
> >mode_info.atom_context);
> +             if (r)
> +                     DRM_ERROR("amdgpu asic init failed\n");
> +     }
>
>       r = amdgpu_resume(adev);
>       if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 21b8cd6..c00f4a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -520,6 +520,20 @@ static int amdgpu_pmops_thaw(struct device *dev)
>       return amdgpu_device_resume(drm_dev, false, true);
>  }
>
> +static int amdgpu_pmops_poweroff(struct device *dev)
> +{
> +     struct pci_dev *pdev = to_pci_dev(dev);
> +     struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +     return amdgpu_device_suspend(drm_dev, true, true);
> +}
> +
> +static int amdgpu_pmops_restore(struct device *dev)
> +{
> +     struct pci_dev *pdev = to_pci_dev(dev);
> +     struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +     return amdgpu_device_resume(drm_dev, false, true);
> +}
> +
>  static int amdgpu_pmops_runtime_suspend(struct device *dev)
>  {
>       struct pci_dev *pdev = to_pci_dev(dev);
> @@ -622,8 +636,8 @@ static const struct dev_pm_ops amdgpu_pm_ops = {
>       .resume = amdgpu_pmops_resume,
>       .freeze = amdgpu_pmops_freeze,
>       .thaw = amdgpu_pmops_thaw,
> -     .poweroff = amdgpu_pmops_freeze,
> -     .restore = amdgpu_pmops_resume,
> +     .poweroff = amdgpu_pmops_poweroff,
> +     .restore = amdgpu_pmops_restore,
>       .runtime_suspend = amdgpu_pmops_runtime_suspend,
>       .runtime_resume = amdgpu_pmops_runtime_resume,
>       .runtime_idle = amdgpu_pmops_runtime_idle,
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx