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@lists.freedesktop.org 抄送: Qu, Jim 主题: RE: [PATCH v2] drm/amd/amdgpu: S4 issue for amdgpu > -----Original Message----- > From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf > Of jimqu > Sent: Saturday, September 03, 2016 2:57 AM > To: amd-gfx@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...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
