[PATCH v2 0/3] Adjust dGPU handling for BACO

[PATCH v2 0/3] Adjust dGPU handling for BACO

[Mario Limonciello]

This series adjusts the handling for dGPUs when the system is going into
s2idle.  The intent is to match the following truth table below:

+-------------------+----------------------------------+----------------------------------+-----------------------------+
|                   | s2idle (no FADT)                 | s2idle (FADT)                    | deep                        |
+-------------------+----------------------------------+----------------------------------+-----------------------------+
| APU Prepare       | 0                                | 0                                | 0                           |
| APU Suspend       | Run                              | Run                              | Run                         |
| BACO dGPU Prepare | 1 if suspended                   | 1 if suspended                   | 1 if suspended              |
| BACO dGPU Suspend | Runtime suspend if prepare was 0 | Runtime suspend if prepare was 0 | S3 suspend if prepare was 0 |
| BOCO dGPU Prepare | 1                                | 1                                | 1 if suspended              |
| BOCO dGPU Suspend | Runtime suspend if prepare was 0 | Runtime suspend if prepare was 0 | S3 suspend if prepare was 0 |
+-------------------+----------------------------------+----------------------------------+-----------------------------+

That is BACO and BOCO are handled very similarly when system is doing s2idle.

v1->v2:
 * Rework flags and flow
 * Try to do runtime suspend first, and if it fails do system suspend

Mario Limonciello (3):
  drm/amd: Allow dGPUs that support BACO to use smart suspend
  drm/amd: Don't always set s3 for dGPUs in all sleep modes
  drm/amd: Add special handling for system s0ix state w/ dGPUs

 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 11 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 44 +++++++++++++++---------
 2 files changed, 34 insertions(+), 21 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] drm/amd: Allow dGPUs that support BACO to use smart suspend

[Mario Limonciello]

If a dGPU is already runtime suspended using BACO, there is no point
to waking it up to run regular suspend callbacks.

Cc: Peter Kopec <pekopec@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Simplify prepare call
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 32 ++++++++++++-------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index e11f83bd1653..750984517192 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2190,8 +2190,9 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
 		/* only need to skip on ATPX */
 		if (amdgpu_device_supports_px(ddev))
 			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
-		/* we want direct complete for BOCO */
-		if (amdgpu_device_supports_boco(ddev))
+		/* we want direct complete for BOCO and for BACO */
+		if (amdgpu_device_supports_boco(ddev) ||
+		    amdgpu_device_supports_baco(ddev))
 			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_SMART_PREPARE |
 						DPM_FLAG_SMART_SUSPEND |
 						DPM_FLAG_MAY_SKIP_RESUME);
@@ -2384,25 +2385,24 @@ static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work)
 	return;
 }
 
+/**
+ * amdgpu_pmops_prepare
+ *
+ * @dev: device pointer
+ *
+ * Run the "prepare" PM operation. For devices supporting
+ * BOCO or BACO use DPM_FLAG_SMART_PREPARE to skip rest of
+ * suspend process.
+ *
+ */
 static int amdgpu_pmops_prepare(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
-	struct amdgpu_device *adev = drm_to_adev(drm_dev);
 
-	/* Return a positive number here so
-	 * DPM_FLAG_SMART_SUSPEND works properly
-	 */
-	if (amdgpu_device_supports_boco(drm_dev))
-		return pm_runtime_suspended(dev);
-
-	/* if we will not support s3 or s2i for the device
-	 *  then skip suspend
-	 */
-	if (!amdgpu_acpi_is_s0ix_active(adev) &&
-	    !amdgpu_acpi_is_s3_active(adev))
-		return 1;
+	if (!dev_pm_test_driver_flags(drm_dev->dev, DPM_FLAG_SMART_PREPARE))
+		return 0;
 
-	return 0;
+	return pm_runtime_suspended(dev);
 }
 
 static void amdgpu_pmops_complete(struct device *dev)
-- 
2.34.1

[PATCH v2 2/3] drm/amd: Don't always set s3 for dGPUs in all sleep modes

[Mario Limonciello]

dGPUs that will be using BACO or BOCO shouldn't be put into S3
when the system is being put into s2idle.

Cc: Peter Kopec <pekopec@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Whitespace
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 25e902077caf..711f2a1bf525 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1038,8 +1038,12 @@ void amdgpu_acpi_detect(void)
  */
 bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
 {
-	return !(adev->flags & AMD_IS_APU) ||
-		(pm_suspend_target_state == PM_SUSPEND_MEM);
+	if (pm_suspend_target_state == PM_SUSPEND_MEM)
+		return true;
+	if (adev->flags & AMD_IS_APU)
+		return false;
+	return !amdgpu_device_supports_baco(&adev->ddev) &&
+		!amdgpu_device_supports_boco(&adev->ddev);
 }
 
 /**
-- 
2.34.1

[PATCH v2 3/3] drm/amd: Add special handling for system s0ix state w/ dGPUs

[Mario Limonciello]

With dGPUs that support BACO or BOCO we want them to go into those
states when the system goes to s2idle.  Detect that the system will
be targeting this state and force the call into runtime suspend.

If the runtime suspend call fails for any reason, then fallback to
standard suspend flow.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * New patch
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |  3 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 12 +++++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 711f2a1bf525..7c3c6380135a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1073,8 +1073,7 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
  */
 bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
 {
-	if (!(adev->flags & AMD_IS_APU) ||
-	    (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
+	if (pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
 		return false;
 
 	if (adev->asic_type < CHIP_RAVEN)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 750984517192..acc032c4c250 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2415,8 +2415,18 @@ static int amdgpu_pmops_suspend(struct device *dev)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct amdgpu_device *adev = drm_to_adev(drm_dev);
 
-	if (amdgpu_acpi_is_s0ix_active(adev))
+	if (amdgpu_acpi_is_s0ix_active(adev)) {
+		/* try to explicitly enter runtime suspend for s2idle on BACO/BOCO */
+		if (dev_pm_test_driver_flags(drm_dev->dev, DPM_FLAG_SMART_SUSPEND)) {
+			int ret;
+
+			ret = pm_runtime_suspend(dev);
+			if (!ret)
+				return 0;
+			DRM_WARN("failed to enter runtime suspend, running system suspend: %d\n", ret);
+		}
 		adev->in_s0ix = true;
+	}
 	else if (amdgpu_acpi_is_s3_active(adev))
 		adev->in_s3 = true;
 	if (!adev->in_s0ix && !adev->in_s3)
-- 
2.34.1

RE: [PATCH v2 0/3] Adjust dGPU handling for BACO

[Quan, Evan]

[AMD Official Use Only - General]



> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Mario Limonciello
> Sent: Tuesday, February 28, 2023 12:43 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Peter Kopec <pekopec@redhat.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH v2 0/3] Adjust dGPU handling for BACO
> 
> This series adjusts the handling for dGPUs when the system is going into
> s2idle.  The intent is to match the following truth table below:
> 
> +-------------------+----------------------------------+----------------------------------
> +-----------------------------+
> |                   | s2idle (no FADT)                 | s2idle (FADT)                    | deep
> |
> +-------------------+----------------------------------+----------------------------------
> +-----------------------------+
> | APU Prepare       | 0                                | 0                                | 0                           |
> | APU Suspend       | Run                              | Run                              | Run
> |
> | BACO dGPU Prepare | 1 if suspended                   | 1 if suspended                   | 1
> if suspended              |
> | BACO dGPU Suspend | Runtime suspend if prepare was 0 | Runtime
> suspend if prepare was 0 | S3 suspend if prepare was 0 |
> | BOCO dGPU Prepare | 1                                | 1                                | 1 if suspended
> |
For BOCO Prepare, it should be also "1 if suspsended" instead of "1" for s2idle per patch1. 
Do I miss anything?

BR
Evan
> | BOCO dGPU Suspend | Runtime suspend if prepare was 0 | Runtime
> suspend if prepare was 0 | S3 suspend if prepare was 0 |
> +-------------------+----------------------------------+----------------------------------
> +-----------------------------+
> 
> That is BACO and BOCO are handled very similarly when system is doing
> s2idle.
> 
> v1->v2:
>  * Rework flags and flow
>  * Try to do runtime suspend first, and if it fails do system suspend
> 
> Mario Limonciello (3):
>   drm/amd: Allow dGPUs that support BACO to use smart suspend
>   drm/amd: Don't always set s3 for dGPUs in all sleep modes
>   drm/amd: Add special handling for system s0ix state w/ dGPUs
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 11 +++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 44 +++++++++++++++-----
> ----
>  2 files changed, 34 insertions(+), 21 deletions(-)
> 
> --
> 2.34.1

RE: [PATCH v2 3/3] drm/amd: Add special handling for system s0ix state w/ dGPUs

[Quan, Evan]

[AMD Official Use Only - General]



> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Mario Limonciello
> Sent: Tuesday, February 28, 2023 12:43 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Peter Kopec <pekopec@redhat.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH v2 3/3] drm/amd: Add special handling for system s0ix state
> w/ dGPUs
> 
> With dGPUs that support BACO or BOCO we want them to go into those
> states when the system goes to s2idle.  Detect that the system will
> be targeting this state and force the call into runtime suspend.
> 
> If the runtime suspend call fails for any reason, then fallback to
> standard suspend flow.
The "standard suspend" means here is normal s2idle flow. Right?
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * New patch
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |  3 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 711f2a1bf525..7c3c6380135a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1073,8 +1073,7 @@ bool amdgpu_acpi_should_gpu_reset(struct
> amdgpu_device *adev)
>   */
>  bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
>  {
> -	if (!(adev->flags & AMD_IS_APU) ||
> -	    (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
> +	if (pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
>  		return false;
> 
>  	if (adev->asic_type < CHIP_RAVEN)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 750984517192..acc032c4c250 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2415,8 +2415,18 @@ static int amdgpu_pmops_suspend(struct device
> *dev)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct amdgpu_device *adev = drm_to_adev(drm_dev);
> 
> -	if (amdgpu_acpi_is_s0ix_active(adev))
> +	if (amdgpu_acpi_is_s0ix_active(adev)) {
> +		/* try to explicitly enter runtime suspend for s2idle on
> BACO/BOCO */
> +		if (dev_pm_test_driver_flags(drm_dev->dev,
> DPM_FLAG_SMART_SUSPEND)) {
> +			int ret;
> +
> +			ret = pm_runtime_suspend(dev);
> +			if (!ret)
> +				return 0;
"ret" seems redundant and can be dropped.

BR
Evan
> +			DRM_WARN("failed to enter runtime suspend,
> running system suspend: %d\n", ret);
> +		}
>  		adev->in_s0ix = true;
> +	}
>  	else if (amdgpu_acpi_is_s3_active(adev))
>  		adev->in_s3 = true;
>  	if (!adev->in_s0ix && !adev->in_s3)
> --
> 2.34.1

Re: [PATCH v2 3/3] drm/amd: Add special handling for system s0ix state w/ dGPUs

[Lazar, Lijo]

On 2/28/2023 10:13 AM, Mario Limonciello wrote:
> With dGPUs that support BACO or BOCO we want them to go into those
> states when the system goes to s2idle.  Detect that the system will
> be targeting this state and force the call into runtime suspend.
> 
> If the runtime suspend call fails for any reason, then fallback to
> standard suspend flow.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>   * New patch
>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |  3 +--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 12 +++++++++++-
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 711f2a1bf525..7c3c6380135a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1073,8 +1073,7 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
>    */
>   bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
>   {
> -	if (!(adev->flags & AMD_IS_APU) ||
> -	    (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
> +	if (pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
>   		return false;
>

This will set adev->in_s0ix flag to be true for all dGPUs. There are 
many places through out suspend/resume logic where it is assumed that 
adev->in_s0ix is set only for APUs. For ex: it skips suspend of GFX 
assuming GFXOFF is a pre-condition for s0ix.

Basically this will break suspend/resume of dGPUs in s2idle if the 
device is not already suspended.

Thanks,
Lijo


>   	if (adev->asic_type < CHIP_RAVEN)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 750984517192..acc032c4c250 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2415,8 +2415,18 @@ static int amdgpu_pmops_suspend(struct device *dev)
>   	struct drm_device *drm_dev = dev_get_drvdata(dev);
>   	struct amdgpu_device *adev = drm_to_adev(drm_dev);
>   
> -	if (amdgpu_acpi_is_s0ix_active(adev))
> +	if (amdgpu_acpi_is_s0ix_active(adev)) {
> +		/* try to explicitly enter runtime suspend for s2idle on BACO/BOCO */
> +		if (dev_pm_test_driver_flags(drm_dev->dev, DPM_FLAG_SMART_SUSPEND)) {
> +			int ret;
> +
> +			ret = pm_runtime_suspend(dev);
> +			if (!ret)
> +				return 0;
> +			DRM_WARN("failed to enter runtime suspend, running system suspend: %d\n", ret);
> +		}
>   		adev->in_s0ix = true;
> +	}
>   	else if (amdgpu_acpi_is_s3_active(adev))
>   		adev->in_s3 = true;
>   	if (!adev->in_s0ix && !adev->in_s3)