All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal
@ 2019-12-26  8:10 Evan Quan
  2019-12-26  8:10 ` [PATCH 2/2] drm/amdgpu: avoid unnecessary SMC firmware reloading Evan Quan
  2019-12-29  6:43 ` [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal Deucher, Alexander
  0 siblings, 2 replies; 5+ messages in thread
From: Evan Quan @ 2019-12-26  8:10 UTC (permalink / raw)
  To: amd-gfx; +Cc: Alexander.Deucher, Evan Quan

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;
+	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);
+}
+
 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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] drm/amdgpu: avoid unnecessary SMC firmware reloading
  2019-12-26  8:10 [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal Evan Quan
@ 2019-12-26  8:10 ` Evan Quan
  2019-12-29  6:44   ` Deucher, Alexander
  2019-12-29  6:43 ` [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal Deucher, Alexander
  1 sibling, 1 reply; 5+ messages in thread
From: Evan Quan @ 2019-12-26  8:10 UTC (permalink / raw)
  To: amd-gfx; +Cc: Alexander.Deucher, Evan Quan

E.g. During non-RAS baco reset, the SMC is still alive.
Thus there is no need to reload the SMC firmware.

Change-Id: I73f6284541d0ca0e82761380a27e32484fb0061c
Signed-off-by: Evan Quan <evan.quan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index c66ca8cc2ebd..ba761e9366e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
@@ -676,6 +676,19 @@ static bool psp_v11_0_compare_sram_data(struct psp_context *psp,
 	return true;
 }
 
+/*
+ * Check whether SMU is still alive. If that's true
+ * (e.g. for non-RAS baco reset), we need to skip SMC firmware reloading.
+ */
+static bool psp_v11_0_smu_reload_quirk(struct psp_context *psp)
+{
+	struct amdgpu_device *adev = psp->adev;
+	uint32_t reg;
+
+	reg = RREG32_PCIE(smnMP1_FIRMWARE_FLAGS | 0x03b00000);
+	return (reg & MP1_FIRMWARE_FLAGS__INTERRUPTS_ENABLED_MASK) ? true : false;
+}
+
 static int psp_v11_0_mode1_reset(struct psp_context *psp)
 {
 	int ret;
@@ -1070,6 +1083,7 @@ static const struct psp_funcs psp_v11_0_funcs = {
 	.ring_stop = psp_v11_0_ring_stop,
 	.ring_destroy = psp_v11_0_ring_destroy,
 	.compare_sram_data = psp_v11_0_compare_sram_data,
+	.smu_reload_quirk = psp_v11_0_smu_reload_quirk,
 	.mode1_reset = psp_v11_0_mode1_reset,
 	.xgmi_get_topology_info = psp_v11_0_xgmi_get_topology_info,
 	.xgmi_set_topology_info = psp_v11_0_xgmi_set_topology_info,
-- 
2.24.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* RE: [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal
  2019-12-26  8:10 [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal Evan Quan
  2019-12-26  8:10 ` [PATCH 2/2] drm/amdgpu: avoid unnecessary SMC firmware reloading Evan Quan
@ 2019-12-29  6:43 ` Deucher, Alexander
  2020-01-02  4:25   ` Quan, Evan
  1 sibling, 1 reply; 5+ messages in thread
From: Deucher, Alexander @ 2019-12-29  6:43 UTC (permalink / raw)
  To: Quan, Evan, amd-gfx

[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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 2/2] drm/amdgpu: avoid unnecessary SMC firmware reloading
  2019-12-26  8:10 ` [PATCH 2/2] drm/amdgpu: avoid unnecessary SMC firmware reloading Evan Quan
@ 2019-12-29  6:44   ` Deucher, Alexander
  0 siblings, 0 replies; 5+ messages in thread
From: Deucher, Alexander @ 2019-12-29  6:44 UTC (permalink / raw)
  To: Quan, Evan, amd-gfx

[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 2/2] drm/amdgpu: avoid unnecessary SMC firmware
> reloading
> 
> E.g. During non-RAS baco reset, the SMC is still alive.
> Thus there is no need to reload the SMC firmware.
> 
> Change-Id: I73f6284541d0ca0e82761380a27e32484fb0061c
> Signed-off-by: Evan Quan <evan.quan@amd.com>


Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index c66ca8cc2ebd..ba761e9366e3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> @@ -676,6 +676,19 @@ static bool psp_v11_0_compare_sram_data(struct
> psp_context *psp,
>  	return true;
>  }
> 
> +/*
> + * Check whether SMU is still alive. If that's true
> + * (e.g. for non-RAS baco reset), we need to skip SMC firmware reloading.
> + */
> +static bool psp_v11_0_smu_reload_quirk(struct psp_context *psp) {
> +	struct amdgpu_device *adev = psp->adev;
> +	uint32_t reg;
> +
> +	reg = RREG32_PCIE(smnMP1_FIRMWARE_FLAGS | 0x03b00000);
> +	return (reg &
> MP1_FIRMWARE_FLAGS__INTERRUPTS_ENABLED_MASK) ? true :
> +false; }
> +
>  static int psp_v11_0_mode1_reset(struct psp_context *psp)  {
>  	int ret;
> @@ -1070,6 +1083,7 @@ static const struct psp_funcs psp_v11_0_funcs = {
>  	.ring_stop = psp_v11_0_ring_stop,
>  	.ring_destroy = psp_v11_0_ring_destroy,
>  	.compare_sram_data = psp_v11_0_compare_sram_data,
> +	.smu_reload_quirk = psp_v11_0_smu_reload_quirk,
>  	.mode1_reset = psp_v11_0_mode1_reset,
>  	.xgmi_get_topology_info = psp_v11_0_xgmi_get_topology_info,
>  	.xgmi_set_topology_info = psp_v11_0_xgmi_set_topology_info,
> --
> 2.24.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal
  2019-12-29  6:43 ` [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal Deucher, Alexander
@ 2020-01-02  4:25   ` Quan, Evan
  0 siblings, 0 replies; 5+ messages in thread
From: Quan, Evan @ 2020-01-02  4:25 UTC (permalink / raw)
  To: Deucher, Alexander, amd-gfx



> -----Original Message-----
> From: Deucher, Alexander <Alexander.Deucher@amd.com>
> Sent: Sunday, December 29, 2019 2:43 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> 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@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?
[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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-01-02  4:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-26  8:10 [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal Evan Quan
2019-12-26  8:10 ` [PATCH 2/2] drm/amdgpu: avoid unnecessary SMC firmware reloading Evan Quan
2019-12-29  6:44   ` Deucher, Alexander
2019-12-29  6:43 ` [PATCH 1/2] drm/amdgpu: handle mp1 state properly on suspend or device removal Deucher, Alexander
2020-01-02  4:25   ` Quan, Evan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.