| Message ID | 1599072130-10043-5-git-send-email-andrey.grodzovsky@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Implement PCI Error Recovery on Navi12 | expand |
On Wed, Sep 02, 2020 at 02:42:06PM -0400, Andrey Grodzovsky wrote: > Cache the PCI state on boot and before each case where we might > loose it. s/loose/lose/ > v2: Add pci_restore_state while caching the PCI state to avoid > breaking PCI core logic for stuff like suspend/resume. > > v3: Extract pci_restore_state from amdgpu_device_cache_pci_state > to avoid superflous restores during GPU resets and suspend/resumes. > > v4: Style fixes. Is the DRM convention to keep the v2/v3/v4 stuff in the commit log? I keep those below the "---" or manually remove them for PCI, but use the local convention, of course. > + /* Have stored pci confspace at hand for restore in sudden PCI error */ I assume that at least from the perspective of this code, all PCI errors are "sudden". Or if they're not, I'm curious about which would be sudden and which would not. > + if (amdgpu_device_cache_pci_state(adev->pdev)) > + pci_restore_state(pdev); > +bool amdgpu_device_cache_pci_state(struct pci_dev *pdev) > +{ > + struct drm_device *dev = pci_get_drvdata(pdev); > + struct amdgpu_device *adev = drm_to_adev(dev); > + int r; > + > + r = pci_save_state(pdev); > + if (!r) { > + kfree(adev->pci_state); > + > + adev->pci_state = pci_store_saved_state(pdev); > + > + if (!adev->pci_state) { > + DRM_ERROR("Failed to store PCI saved state"); > + return false; > + } > + } else { > + DRM_WARN("Failed to save PCI state, err:%d\n", r); > + return false; > + } > + > + return true; > +} > + > +bool amdgpu_device_load_pci_state(struct pci_dev *pdev) > +{ > + struct drm_device *dev = pci_get_drvdata(pdev); > + struct amdgpu_device *adev = drm_to_adev(dev); > + int r; > + > + if (!adev->pci_state) > + return false; > + > + r = pci_load_saved_state(pdev, adev->pci_state); I'm a little bit hesitant to pci_load_saved_state() and pci_store_saved_state() being used here, simply because they're currently only used by VFIO, Xen, and nvme. So I don't have a real objection, but just pointing out that apparently you're doing something really special that isn't commonly used and tested, so it's more likely to be broken or incomplete. There's lots of state that the PCI core *can't* save/restore, and pci_load_saved_state() doesn't even handle all the architected PCI state, i.e., we only call pci_add_cap_save_buffer() or pci_add_ext_cap_save_buffer() for a few of the capabilities.
On 9/2/20 6:23 PM, Bjorn Helgaas wrote: > On Wed, Sep 02, 2020 at 02:42:06PM -0400, Andrey Grodzovsky wrote: >> Cache the PCI state on boot and before each case where we might >> loose it. > s/loose/lose/ > >> v2: Add pci_restore_state while caching the PCI state to avoid >> breaking PCI core logic for stuff like suspend/resume. >> >> v3: Extract pci_restore_state from amdgpu_device_cache_pci_state >> to avoid superflous restores during GPU resets and suspend/resumes. >> >> v4: Style fixes. > Is the DRM convention to keep the v2/v3/v4 stuff in the commit log? I > keep those below the "---" or manually remove them for PCI, but use > the local convention, of course. > >> + /* Have stored pci confspace at hand for restore in sudden PCI error */ > I assume that at least from the perspective of this code, all PCI > errors are "sudden". Or if they're not, I'm curious about which would > be sudden and which would not. Yes, all PCI errors are sudden from drivers point of view indeed, I am just stressing the fact that i have to 'plan ahead' and store working PCI config because to my understating by the time when we are notified of the PCI error in err_detected callback the device and it's PCI confspace registers are already inaccessible and so it's to late to try and save the confspace at this time. > >> + if (amdgpu_device_cache_pci_state(adev->pdev)) >> + pci_restore_state(pdev); >> +bool amdgpu_device_cache_pci_state(struct pci_dev *pdev) >> +{ >> + struct drm_device *dev = pci_get_drvdata(pdev); >> + struct amdgpu_device *adev = drm_to_adev(dev); >> + int r; >> + >> + r = pci_save_state(pdev); >> + if (!r) { >> + kfree(adev->pci_state); >> + >> + adev->pci_state = pci_store_saved_state(pdev); >> + >> + if (!adev->pci_state) { >> + DRM_ERROR("Failed to store PCI saved state"); >> + return false; >> + } >> + } else { >> + DRM_WARN("Failed to save PCI state, err:%d\n", r); >> + return false; >> + } >> + >> + return true; >> +} >> + >> +bool amdgpu_device_load_pci_state(struct pci_dev *pdev) >> +{ >> + struct drm_device *dev = pci_get_drvdata(pdev); >> + struct amdgpu_device *adev = drm_to_adev(dev); >> + int r; >> + >> + if (!adev->pci_state) >> + return false; >> + >> + r = pci_load_saved_state(pdev, adev->pci_state); > I'm a little bit hesitant to pci_load_saved_state() and > pci_store_saved_state() being used here, simply because they're > currently only used by VFIO, Xen, and nvme. So I don't have a real > objection, but just pointing out that apparently you're doing > something really special that isn't commonly used and tested, so it's > more likely to be broken or incomplete. See my assumption above, I assume I have to explicitly save (cache) the PCI state while the device is fully operational since I assume it's not possible when PCI error occurs and DPC isolates the device. I do spot PCI core calling pci_dev_save_and_disable and pci_dev_restore as part of pcie_do_recovery->pci_reset_bus and those functions do store and restore the PCI confpsace but I am not sure this code executes in all cases and pcie_do_recovery->pci_reset_bus was just added now by last sathyanarayanan's patch he gave me anyway... Andrey > > There's lots of state that the PCI core *can't* save/restore, and > pci_load_saved_state() doesn't even handle all the architected PCI > state, i.e., we only call pci_add_cap_save_buffer() or > pci_add_ext_cap_save_buffer() for a few of the capabilities.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index b20354f..13f92de 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -992,7 +992,9 @@ struct amdgpu_device { atomic_t throttling_logging_enabled; struct ratelimit_state throttling_logging_rs; uint32_t ras_features; + bool in_pci_err_recovery; + struct pci_saved_state *pci_state; }; static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) @@ -1272,6 +1274,9 @@ pci_ers_result_t amdgpu_pci_mmio_enabled(struct pci_dev *pdev); pci_ers_result_t amdgpu_pci_slot_reset(struct pci_dev *pdev); void amdgpu_pci_resume(struct pci_dev *pdev); +bool amdgpu_device_cache_pci_state(struct pci_dev *pdev); +bool amdgpu_device_load_pci_state(struct pci_dev *pdev); + #include "amdgpu_object.h" /* used by df_v3_6.c and amdgpu_pmu.c */ diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 412d07e..174e09b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1283,7 +1283,7 @@ static void amdgpu_switcheroo_set_state(struct pci_dev *pdev, dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; pci_set_power_state(dev->pdev, PCI_D0); - pci_restore_state(dev->pdev); + amdgpu_device_load_pci_state(dev->pdev); r = pci_enable_device(dev->pdev); if (r) DRM_WARN("pci_enable_device failed (%d)\n", r); @@ -1296,7 +1296,7 @@ static void amdgpu_switcheroo_set_state(struct pci_dev *pdev, drm_kms_helper_poll_disable(dev); dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; amdgpu_device_suspend(dev, true); - pci_save_state(dev->pdev); + amdgpu_device_cache_pci_state(dev->pdev); /* Shut down the device */ pci_disable_device(dev->pdev); pci_set_power_state(dev->pdev, PCI_D3cold); @@ -3399,6 +3399,10 @@ int amdgpu_device_init(struct amdgpu_device *adev, if (r) dev_err(adev->dev, "amdgpu_pmu_init failed\n"); + /* Have stored pci confspace at hand for restore in sudden PCI error */ + if (amdgpu_device_cache_pci_state(adev->pdev)) + pci_restore_state(pdev); + return 0; failed: @@ -3423,6 +3427,8 @@ void amdgpu_device_fini(struct amdgpu_device *adev) flush_delayed_work(&adev->delayed_init_work); adev->shutdown = true; + kfree(adev->pci_state); + /* make sure IB test finished before entering exclusive mode * to avoid preemption on IB test * */ @@ -4847,7 +4853,7 @@ pci_ers_result_t amdgpu_pci_slot_reset(struct pci_dev *pdev) /* wait for asic to come out of reset */ msleep(500); - pci_restore_state(pdev); + amdgpu_device_load_pci_state(pdev); /* confirm ASIC came out of reset */ for (i = 0; i < adev->usec_timeout; i++) { @@ -4927,6 +4933,9 @@ pci_ers_result_t amdgpu_pci_slot_reset(struct pci_dev *pdev) out: if (!r) { + if (amdgpu_device_cache_pci_state(adev->pdev)) + pci_restore_state(adev->pdev); + DRM_INFO("PCIe error recovery succeeded\n"); } else { DRM_ERROR("PCIe error recovery failed, err:%d", r); @@ -4966,3 +4975,50 @@ void amdgpu_pci_resume(struct pci_dev *pdev) amdgpu_device_unlock_adev(adev); } + +bool amdgpu_device_cache_pci_state(struct pci_dev *pdev) +{ + struct drm_device *dev = pci_get_drvdata(pdev); + struct amdgpu_device *adev = drm_to_adev(dev); + int r; + + r = pci_save_state(pdev); + if (!r) { + kfree(adev->pci_state); + + adev->pci_state = pci_store_saved_state(pdev); + + if (!adev->pci_state) { + DRM_ERROR("Failed to store PCI saved state"); + return false; + } + } else { + DRM_WARN("Failed to save PCI state, err:%d\n", r); + return false; + } + + return true; +} + +bool amdgpu_device_load_pci_state(struct pci_dev *pdev) +{ + struct drm_device *dev = pci_get_drvdata(pdev); + struct amdgpu_device *adev = drm_to_adev(dev); + int r; + + if (!adev->pci_state) + return false; + + r = pci_load_saved_state(pdev, adev->pci_state); + + if (!r) { + pci_restore_state(pdev); + } else { + DRM_WARN("Failed to load PCI state, err:%d\n", r); + return false; + } + + return true; +} + + diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index af99a5d..b653c72 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1322,7 +1322,7 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev) if (amdgpu_is_atpx_hybrid()) { pci_ignore_hotplug(pdev); } else { - pci_save_state(pdev); + amdgpu_device_cache_pci_state(pdev); pci_disable_device(pdev); pci_ignore_hotplug(pdev); pci_set_power_state(pdev, PCI_D3cold); @@ -1355,7 +1355,7 @@ static int amdgpu_pmops_runtime_resume(struct device *dev) pci_set_master(pdev); } else { pci_set_power_state(pdev, PCI_D0); - pci_restore_state(pdev); + amdgpu_device_load_pci_state(pdev); ret = pci_enable_device(pdev); if (ret) return ret; diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c index 4d14023..0ec6603 100644 --- a/drivers/gpu/drm/amd/amdgpu/nv.c +++ b/drivers/gpu/drm/amd/amdgpu/nv.c @@ -311,7 +311,7 @@ static int nv_asic_mode1_reset(struct amdgpu_device *adev) /* disable BM */ pci_clear_master(adev->pdev); - pci_save_state(adev->pdev); + amdgpu_device_cache_pci_state(adev->pdev); if (amdgpu_dpm_is_mode1_reset_supported(adev)) { dev_info(adev->dev, "GPU smu mode1 reset\n"); @@ -323,7 +323,7 @@ static int nv_asic_mode1_reset(struct amdgpu_device *adev) if (ret) dev_err(adev->dev, "GPU mode1 reset failed\n"); - pci_restore_state(adev->pdev); + amdgpu_device_load_pci_state(adev->pdev); /* wait for asic to come out of reset */ for (i = 0; i < adev->usec_timeout; i++) { diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c index 2f93c47..ddd55e3 100644 --- a/drivers/gpu/drm/amd/amdgpu/soc15.c +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c @@ -484,13 +484,13 @@ static int soc15_asic_mode1_reset(struct amdgpu_device *adev) /* disable BM */ pci_clear_master(adev->pdev); - pci_save_state(adev->pdev); + amdgpu_device_cache_pci_state(adev->pdev); ret = psp_gpu_reset(adev); if (ret) dev_err(adev->dev, "GPU mode1 reset failed\n"); - pci_restore_state(adev->pdev); + amdgpu_device_load_pci_state(adev->pdev); /* wait for asic to come out of reset */ for (i = 0; i < adev->usec_timeout; i++) {
Cache the PCI state on boot and before each case where we might loose it. v2: Add pci_restore_state while caching the PCI state to avoid breaking PCI core logic for stuff like suspend/resume. v3: Extract pci_restore_state from amdgpu_device_cache_pci_state to avoid superflous restores during GPU resets and suspend/resumes. v4: Style fixes. Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 5 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 62 ++++++++++++++++++++++++++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +- drivers/gpu/drm/amd/amdgpu/nv.c | 4 +- drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +- 5 files changed, 70 insertions(+), 9 deletions(-)