> -----Original Message-----
> From: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Sent: Sunday, December 29, 2019 2:43 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend
> or device removal
>
> [AMD Public Use]
>
> > -----Original Message-----
> > From: Quan, Evan <Evan.Quan@xxxxxxx>
> > Sent: Thursday, December 26, 2019 3:11 AM
> > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Quan, Evan
> > <Evan.Quan@xxxxxxx>
> > Subject: [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or
> > device removal
> >
> > Set mp1 state properly for non gpu reset cases.
> >
> > Change-Id: I2a007910da6b5e2d1f8915d17827621ebd2e8c59
> > Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 16 ++++++++++++++--
> > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 13 +++++++++++++
> > 3 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index d36245321cb0..739ff4e4e845 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -1089,6 +1089,7 @@ static int amdgpu_device_check_arguments(struct
> > amdgpu_device *adev) static void amdgpu_switcheroo_set_state(struct
> > pci_dev *pdev, enum vga_switcheroo_state state) {
> > struct drm_device *dev = pci_get_drvdata(pdev);
> > + struct amdgpu_device *adev = dev->dev_private;
> > int r;
> >
> > if (amdgpu_device_supports_boco(dev) && state ==
> > VGA_SWITCHEROO_OFF) @@ -1112,7 +1113,9 @@ static void
> > amdgpu_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero
> > pr_info("amdgpu: switched off\n");
> > drm_kms_helper_poll_disable(dev);
> > dev->switch_power_state =
> > DRM_SWITCH_POWER_CHANGING;
> > + adev->mp1_state = PP_MP1_STATE_SHUTDOWN;
> > amdgpu_device_suspend(dev, true);
> > + adev->mp1_state = PP_MP1_STATE_NONE;
> > pci_save_state(dev->pdev);
> > /* Shut down the device */
> > pci_disable_device(dev->pdev);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 61dc26515c7e..13278f1c1b14 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -1157,8 +1157,14 @@ amdgpu_pci_shutdown(struct pci_dev *pdev)
> > static int amdgpu_pmops_suspend(struct device *dev) {
> > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > + struct amdgpu_device *adev = drm_dev->dev_private;
> > + int r;
> > +
> > + adev->mp1_state = PP_MP1_STATE_SHUTDOWN;
> > + r = amdgpu_device_suspend(drm_dev, true);
> > + adev->mp1_state = PP_MP1_STATE_NONE;
> >
> > - return amdgpu_device_suspend(drm_dev, true);
> > + return r;
> > }
> >
> > static int amdgpu_pmops_resume(struct device *dev) @@ -1198,8 +1204,14
> > @@ static int amdgpu_pmops_thaw(struct device *dev) static int
> > amdgpu_pmops_poweroff(struct device *dev) {
> > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > + struct amdgpu_device *adev = drm_dev->dev_private;
> > + int r;
> > +
> > + adev->mp1_state = PP_MP1_STATE_SHUTDOWN;
>
> Should this be STATE_UNLOAD for poweroff?
[Quan, Evan] "PrepareMp1ForUnload is for PnP cases, where ASIC is not reset. If the whole ASIC is reset or need to shutdown, then we can simply use PrepareMp1ForShutdown." - a quote from a SMC firmware developer.
According to that, I think PrepareMp1ForShutdown is more proper. How do you think?
>
> > + r = amdgpu_device_suspend(drm_dev, true);
> > + adev->mp1_state = PP_MP1_STATE_NONE;
> >
> > - return amdgpu_device_suspend(drm_dev, true);
> > + return r;
> > }
> >
> > static int amdgpu_pmops_restore(struct device *dev) diff --git
> > a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > index d07c4f2ccee7..4a7fb6b15635 100644
> > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > @@ -1370,6 +1370,18 @@ int smu_reset(struct smu_context *smu)
> > return ret;
> > }
> >
> > +void smu_late_fini(void *handle)
> > +{
> > + struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > + struct smu_context *smu = &adev->smu;
> > +
> > + /*
> > + * Put the mp1 in the right state.
> > + * This gets called on driver unloading.
> > + */
> > + smu_send_smc_msg(smu, SMU_MSG_PrepareMp1ForShutdown); }
> > +
>
> Do we need something similar for the powerplay code? E.g., pp_late_fini()?
> Also shouldn't this be adev->mp1_state or prepare for unload rather than
> hardcoding it to prepare for shutdown?
[Quan, Evan] This is specifically for the amdgpu_driver_unload_kms() use case.
For that, the call path is amdgpu_driver_unload_kms() -> amdgpu_device_fini() -> amdgpu_device_ip_fini() -> hw_fini().
And the suspend routines are not on the call path. So, the adev->mp1_state way is not workable.
Yes, I think powerplay code needs this also.
Maybe we can use prepareMp1forunload. Considering amdgpu_driver_unload_kms() may be called on runtime driver/module unloading and
the ASIC needs no power down or reset for that.
>
> Alex
>
> > static int smu_suspend(void *handle)
> > {
> > int ret;
> > @@ -1931,6 +1943,7 @@ const struct amd_ip_funcs smu_ip_funcs = {
> > .sw_fini = smu_sw_fini,
> > .hw_init = smu_hw_init,
> > .hw_fini = smu_hw_fini,
> > + .late_fini = smu_late_fini,
> > .suspend = smu_suspend,
> > .resume = smu_resume,
> > .is_idle = NULL,
> > --
> > 2.24.0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
