RE: [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal

["Deucher, Alexander" <Alexander.Deucher@amd.com>]

[AMD Public Use]

> -----Original Message-----
> From: Quan, Evan <Evan.Quan@amd.com>
> Sent: Thursday, December 26, 2019 3:11 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>
> 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@amd.com>
> ---
>  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?

> +	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?

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx