[v2] drm/amd/amdgpu: S4 issue for amdgpu

Submitted by Qu, Jim on Sept. 3, 2016, 6:56 a.m.

Details

Message ID 1472885811-5210-1-git-send-email-Jim.Qu@amd.com
State New
Headers show
Series "drm/amd/amdgpu: S4 issue for amdgpu" ( rev: 2 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Qu, Jim Sept. 3, 2016, 6:56 a.m.
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.Qu@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(-)

Patch hide | download patch | download mbox

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");
 	}
 
 	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,

Comments

On Saturday, September 3, 2016 2:56:51 PM EDT jimqu wrote:
> 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.Qu@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(-)
> 

Seems ok on CIK here with 4.8-rc5 + drm-next-4.9-wip merged in, I usually 
hibernate and reboot into Windows sometimes. I ran pm-hibernate manually.

When you set:
echo reboot > /sys/power/disk

Tested By: Shawn Starr <shawn.starr@rogers.com>

> 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");
>  	}
> 
>  	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,
Am 07.09.2016 um 10:08 schrieb Shawn Starr:
> On Saturday, September 3, 2016 2:56:51 PM EDT jimqu wrote:
>> 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.Qu@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(-)
>>
> Seems ok on CIK here with 4.8-rc5 + drm-next-4.9-wip merged in, I usually
> hibernate and reboot into Windows sometimes. I ran pm-hibernate manually.
>
> When you set:
> echo reboot > /sys/power/disk
>
> Tested By: Shawn Starr <shawn.starr@rogers.com>

I unfortunately don't have the slightest idea how this part of PM works 
and Alex is on vacation. But it sounds important so let's try to get 
this into out branches ASAP.

One minor nit pick below, apart from that looks good to me.

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

Please add an empty line between the declaration and code.

With that fixed the patch is Acked-by: Christian König 
<christian.koenig@amd.com>.

Regards,
Christian.

>> +	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,
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Thanks to guys, I will correct and push it.

Thanks
JimQu

________________________________________
发件人: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> 代表 Christian König <deathsimple@vodafone.de>
发送时间: 2016年9月7日 16:15:03
收件人: Shawn Starr; amd-gfx@lists.freedesktop.org
抄送: Qu, Jim
主题: Re: [PATCH v2] drm/amd/amdgpu: S4 issue for amdgpu

Am 07.09.2016 um 10:08 schrieb Shawn Starr:
> On Saturday, September 3, 2016 2:56:51 PM EDT jimqu wrote:

>> 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.Qu@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(-)

>>

> Seems ok on CIK here with 4.8-rc5 + drm-next-4.9-wip merged in, I usually

> hibernate and reboot into Windows sometimes. I ran pm-hibernate manually.

>

> When you set:

> echo reboot > /sys/power/disk

>

> Tested By: Shawn Starr <shawn.starr@rogers.com>


I unfortunately don't have the slightest idea how this part of PM works
and Alex is on vacation. But it sounds important so let's try to get
this into out branches ASAP.

One minor nit pick below, apart from that looks good to me.

>

>> 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");

>>      }

>>

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


Please add an empty line between the declaration and code.

With that fixed the patch is Acked-by: Christian König
<christian.koenig@amd.com>.

Regards,
Christian.

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

>

> _______________________________________________

> 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
> -----Original Message-----

> From: amd-gfx [mailto:amd-gfx-bounces@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.Qu@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
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-bounces@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.Qu@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
> -----Original Message-----

> From: Qu, Jim

> Sent: Thursday, September 08, 2016 4:51 AM

> To: Deucher, Alexander; amd-gfx@lists.freedesktop.org

> Subject: 答复: [PATCH v2] drm/amd/amdgpu: S4 issue for amdgpu

> 

> 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.


I don't think it will make a difference since the GPU will be powered down anyway via ACPI, but it's worth checking.

> 

> BTW, driver dpm does not work when resume back after hibernation on CI.


We may need to adjust the SMC setup logic to see if the firmware is already loaded or not similar to what we do for VI.  I'm not too familiar with how the SMC interacts with a GPU reset since the SMC is the one actually doing the reset on CI and newer.

Alex

> 

> 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-bounces@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.Qu@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