Skip to contentSkip to conversation

drm/amd: Invert APU check for amdgpu_device_evict_resources()

PATCH Superseded Mario Limonciello 2 participants 8 messages Nov 24, 2024 – Nov 26, 2024

Subsystems (2)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v4 1/2 drm/amd: Invert APU check for amdgpu_device_evict_resources()

From: Mario Limonciello <[email protected]>

Resource eviction isn't needed for s3 or s2idle on APUs, but should be run for S4. As amdgpu_device_evict_resources() will be called by prepare notifier adjust logic so that APUs only cover S4.

--
v2:
* New patch

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +2 -2
index 0171d240fcb05..996e9c78384dd 100644
@@ -4735,8 +4735,8 @@ static int amdgpu_device_evict_resources(struct amdgpu_device *adev)
 {
 	int ret;
 
-	/* No need to evict vram on APUs for suspend to ram or s2idle */
-	if ((adev->in_s3 || adev->in_s0ix) && (adev->flags & AMD_IS_APU))
+	/* No need to evict vram on APUs unless going to S4 */
+	if (!adev->in_s4 && (adev->flags & AMD_IS_APU))
 		return 0;
 
 	ret = amdgpu_ttm_evict_resources(adev, TTM_PL_VRAM);
Signature · 1 line
2.43.0
#1 lore raw

PATCH v4 2/2 drm/amd: Add Suspend/Hibernate notification callback support

From: Mario Limonciello <[email protected]>

As part of the suspend sequence VRAM needs to be evicted on dGPUs. In order to make suspend/resume more reliable we moved this into the pmops prepare() callback so that the suspend sequence would fail but the system could remain operational under high memory usage suspend.

Another class of issues exist though where due to memory fragementation there isn't a large enough contiguous space and swap isn't accessible.

Add support for a suspend/hibernate notification callback that could evict VRAM before tasks are frozen. This should allow paging out to swap if necessary.

---
v4:
* Make non fatal, drop patch 3

---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 45 ++++++++++++++++++++++
 2 files changed, 46 insertions(+)
drivers/gpu/drm/amd/amdgpu/amdgpu.h +1 -0
index d8bc6da500161..79ec4ab8ecfc5 100644
@@ -879,6 +879,7 @@ struct amdgpu_device {
 	bool				need_swiotlb;
 	bool				accel_working;
 	struct notifier_block		acpi_nb;
+	struct notifier_block		pm_nb;
 	struct amdgpu_i2c_chan		*i2c_bus[AMDGPU_MAX_I2C_BUS];
 	struct debugfs_blob_wrapper     debugfs_vbios_blob;
 	struct debugfs_blob_wrapper     debugfs_discovery_blob;
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +45 -0

69 lines not on this page · raw message

Signature · 1 line
2.43.0
#2 lore raw

Re: [PATCH v4 2/2] drm/amd: Add Suspend/Hibernate notification callback support

Quoted 87 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

As part of the suspend sequence VRAM needs to be evicted on dGPUs. In order to make suspend/resume more reliable we moved this into the pmops prepare() callback so that the suspend sequence would fail but the system could remain operational under high memory usage suspend.

Another class of issues exist though where due to memory fragementation there isn't a large enough contiguous space and swap isn't accessible.

Add support for a suspend/hibernate notification callback that could evict VRAM before tasks are frozen. This should allow paging out to swap if necessary.

---
v4:
* Make non fatal, drop patch 3

---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 45 ++++++++++++++++++++++
 2 files changed, 46 insertions(+)
drivers/gpu/drm/amd/amdgpu/amdgpu.h +1 -0
index d8bc6da500161..79ec4ab8ecfc5 100644
@@ -879,6 +879,7 @@ struct amdgpu_device {
 	bool				need_swiotlb;
 	bool				accel_working;
 	struct notifier_block		acpi_nb;
+	struct notifier_block		pm_nb;
 	struct amdgpu_i2c_chan		*i2c_bus[AMDGPU_MAX_I2C_BUS];
 	struct debugfs_blob_wrapper     debugfs_vbios_blob;
 	struct debugfs_blob_wrapper     debugfs_discovery_blob;
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +19 -0
index 996e9c78384dd..56510ab4b6650 100644
@@ -191,6 +191,8 @@ void amdgpu_set_init_level(struct amdgpu_device *adev,
 }
 
 static inline void amdgpu_device_stop_pending_resets(struct amdgpu_device *adev);
+static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mode,
+				     void *data);
 
 /**
  * DOC: pcie_replay_count
@@ -4553,6 +4555,11 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 
 	amdgpu_device_check_iommu_direct_map(adev);
 
+	adev->pm_nb.notifier_call = amdgpu_device_pm_notifier;
+	r = register_pm_notifier(&adev->pm_nb);
+	if (r)
+		goto failed;
+
 	return 0;
 
 release_ras_con:
@@ -4617,6 +4624,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 		drain_workqueue(adev->mman.bdev.wq);
 	adev->shutdown = true;
 
+	unregister_pm_notifier(&adev->pm_nb);
+
 	/* make sure IB test finished before entering exclusive mode
 	 * to avoid preemption on IB test
 	 */
@@ -4748,6 +4757,42 @@ static int amdgpu_device_evict_resources(struct amdgpu_device *adev)
 /*
  * Suspend & resume.
  */
+/**
+ * amdgpu_device_pm_notifier - Notification block for Suspend/Hibernate events
+ * @nb: notifier block
+ * @mode: suspend mode
+ * @data: data
+ *
+ * This function is called when the system is about to suspend or hibernate.
+ * It is used to evict resources from the device before the system goes to
+ * sleep while there is still access to swap.
+ */

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c static int amdgpu_device_evict_resources(struct amdgpu_device *adev) · line 4779

Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +10 -0
+static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mode,
+				     void *data)
+{
+	struct amdgpu_device *adev = container_of(nb, struct amdgpu_device, pm_nb);
+	int r;
+
+	switch (mode) {
+	case PM_HIBERNATION_PREPARE:
+		adev->in_s4 = true;
+		fallthrough;

Based on https://gitlab.freedesktop.org/drm/amd/-/issues/3781

What if this callback takes care only of suspend case and leaves the hibernate case to dpm_prepare callback?

Thanks,
Lijo

Quoted 19 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +16 -0
+	case PM_SUSPEND_PREPARE:
+		r = amdgpu_device_evict_resources(adev);
+		adev->in_s4 = false;
+		/*
+		 * This is considered non-fatal at thie time because
+		 * amdgpu_device_prepare() will also evict resources.
+		 * See https://gitlab.freedesktop.org/drm/amd/-/issues/3781
+		 */
+		if (r)
+			drm_warn(adev_to_drm(adev), "Failed to evict resources, freeze active processes if problems occur\n");
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 /**
  * amdgpu_device_prepare - prepare for device suspend
  *
#3 lore raw

Re: [PATCH v4 2/2] drm/amd: Add Suspend/Hibernate notification callback support

Quoted 103 lines · Lijo
Quoted 87 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

As part of the suspend sequence VRAM needs to be evicted on dGPUs. In order to make suspend/resume more reliable we moved this into the pmops prepare() callback so that the suspend sequence would fail but the system could remain operational under high memory usage suspend.

Another class of issues exist though where due to memory fragementation there isn't a large enough contiguous space and swap isn't accessible.

Add support for a suspend/hibernate notification callback that could evict VRAM before tasks are frozen. This should allow paging out to swap if necessary.

---
v4:
* Make non fatal, drop patch 3

---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 45 ++++++++++++++++++++++
  2 files changed, 46 insertions(+)
drivers/gpu/drm/amd/amdgpu/amdgpu.h +1 -0
index d8bc6da500161..79ec4ab8ecfc5 100644
@@ -879,6 +879,7 @@ struct amdgpu_device {
  	bool				need_swiotlb;
  	bool				accel_working;
  	struct notifier_block		acpi_nb;
+	struct notifier_block		pm_nb;
  	struct amdgpu_i2c_chan		*i2c_bus[AMDGPU_MAX_I2C_BUS];
  	struct debugfs_blob_wrapper     debugfs_vbios_blob;
  	struct debugfs_blob_wrapper     debugfs_discovery_blob;
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +19 -0
index 996e9c78384dd..56510ab4b6650 100644
@@ -191,6 +191,8 @@ void amdgpu_set_init_level(struct amdgpu_device *adev,
  }
 
  static inline void amdgpu_device_stop_pending_resets(struct amdgpu_device *adev);
+static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mode,
+				     void *data);
 
  /**
   * DOC: pcie_replay_count
@@ -4553,6 +4555,11 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 
  	amdgpu_device_check_iommu_direct_map(adev);
 
+	adev->pm_nb.notifier_call = amdgpu_device_pm_notifier;
+	r = register_pm_notifier(&adev->pm_nb);
+	if (r)
+		goto failed;
+
  	return 0;
 
  release_ras_con:
@@ -4617,6 +4624,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
  		drain_workqueue(adev->mman.bdev.wq);
  	adev->shutdown = true;
 
+	unregister_pm_notifier(&adev->pm_nb);
+
  	/* make sure IB test finished before entering exclusive mode
  	 * to avoid preemption on IB test
  	 */
@@ -4748,6 +4757,42 @@ static int amdgpu_device_evict_resources(struct amdgpu_device *adev)
  /*
   * Suspend & resume.
   */
+/**
+ * amdgpu_device_pm_notifier - Notification block for Suspend/Hibernate events
+ * @nb: notifier block
+ * @mode: suspend mode
+ * @data: data
+ *
+ * This function is called when the system is about to suspend or hibernate.
+ * It is used to evict resources from the device before the system goes to
+ * sleep while there is still access to swap.
+ */

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c static int amdgpu_device_evict_resources(struct amdgpu_device *adev) · line 4779

Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +10 -0
+static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mode,
+				     void *data)
+{
+	struct amdgpu_device *adev = container_of(nb, struct amdgpu_device, pm_nb);
+	int r;
+
+	switch (mode) {
+	case PM_HIBERNATION_PREPARE:
+		adev->in_s4 = true;
+		fallthrough;

Based on https://gitlab.freedesktop.org/drm/amd/-/issues/3781

Quoted 2 lines

What if this callback takes care only of suspend case and leaves the hibernate case to dpm_prepare callback?

Then hibernate would fail under memory pressure.

My take is this failure with hibernate is a userspace problem (whether userspace decides to freeze the tasks or not). I think it's better that we /try/ to do the eviction and notify them if userspace should be changed.

Quoted 23 lines

Thanks,
Lijo

Quoted 19 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +16 -0
+	case PM_SUSPEND_PREPARE:
+		r = amdgpu_device_evict_resources(adev);
+		adev->in_s4 = false;
+		/*
+		 * This is considered non-fatal at thie time because
+		 * amdgpu_device_prepare() will also evict resources.
+		 * See https://gitlab.freedesktop.org/drm/amd/-/issues/3781
+		 */
+		if (r)
+			drm_warn(adev_to_drm(adev), "Failed to evict resources, freeze active processes if problems occur\n");
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
  /**
   * amdgpu_device_prepare - prepare for device suspend
   *
#4 lore raw

Re: [PATCH v4 2/2] drm/amd: Add Suspend/Hibernate notification callback support

Quoted 113 lines · Mario Limonciello
Quoted 110 lines · Lijo
Quoted 96 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

As part of the suspend sequence VRAM needs to be evicted on dGPUs. In order to make suspend/resume more reliable we moved this into the pmops prepare() callback so that the suspend sequence would fail but the system could remain operational under high memory usage suspend.

Another class of issues exist though where due to memory fragementation there isn't a large enough contiguous space and swap isn't accessible.

Add support for a suspend/hibernate notification callback that could evict VRAM before tasks are frozen. This should allow paging out to swap if necessary.

---
v4:
  * Make non fatal, drop patch 3
---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 + drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 45 ++++++++++++++++++++++

  2 files changed, 46 insertions(+)
drivers/gpu/drm/ renamed from drivers/gpu/drm/amd/amdgpu/amdgpu.h +0 -0

amd/amdgpu/amdgpu.h
index d8bc6da500161..79ec4ab8ecfc5 100644

drivers/gpu/drm/amd/amdgpu/amdgpu.h +1 -0
@@ -879,6 +879,7 @@ struct amdgpu_device {
       bool                need_swiotlb;
       bool                accel_working;
       struct notifier_block        acpi_nb;
+    struct notifier_block        pm_nb;
       struct amdgpu_i2c_chan        *i2c_bus[AMDGPU_MAX_I2C_BUS];
       struct debugfs_blob_wrapper     debugfs_vbios_blob;
       struct debugfs_blob_wrapper     debugfs_discovery_blob;
drivers/ renamed from drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +0 -0

gpu/drm/amd/amdgpu/amdgpu_device.c
index 996e9c78384dd..56510ab4b6650 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +0 -0
@@ -191,6 +191,8 @@ void amdgpu_set_init_level(struct amdgpu_device
 *adev,
   }

    static inline void amdgpu_device_stop_pending_resets(struct amdgpu_device *adev);
+static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mode,
+                     void *data);
    /**
   * DOC: pcie_replay_count

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +4 -0
@@ -4553,6 +4555,11 @@ int amdgpu_device_init(struct amdgpu_device
 *adev,
         amdgpu_device_check_iommu_direct_map(adev);
   +    adev->pm_nb.notifier_call = amdgpu_device_pm_notifier;
+    r = register_pm_notifier(&adev->pm_nb);
+    if (r)
+        goto failed;
+
       return 0;

    release_ras_con:

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +1 -0
@@ -4617,6 +4624,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device
 *adev)
           drain_workqueue(adev->mman.bdev.wq);
       adev->shutdown = true;
   +    unregister_pm_notifier(&adev->pm_nb);
+
       /* make sure IB test finished before entering exclusive mode
        * to avoid preemption on IB test

       */

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +2 -0
@@ -4748,6 +4757,42 @@ static int
 amdgpu_device_evict_resources(struct amdgpu_device *adev)
   /*
    * Suspend & resume.
    */
+/**
+ * amdgpu_device_pm_notifier - Notification block for Suspend/

Hibernate events
+ * @nb: notifier block
+ * @mode: suspend mode
+ * @data: data
+ *
+ * This function is called when the system is about to suspend or hibernate.
+ * It is used to evict resources from the device before the system goes to
+ * sleep while there is still access to swap.
+ */
+static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mode,
+                     void *data)
+{

Quoted 8 lines

+    struct amdgpu_device *adev = container_of(nb, struct amdgpu_device, pm_nb);
+    int r;
+
+    switch (mode) {
+    case PM_HIBERNATION_PREPARE:
+        adev->in_s4 = true;
+        fallthrough;

Based on https://gitlab.freedesktop.org/drm/amd/-/issues/3781

Quoted 9 lines
Quoted 2 lines

What if this callback takes care only of suspend case and leaves the hibernate case to dpm_prepare callback?

Then hibernate would fail under memory pressure.

My take is this failure with hibernate is a userspace problem (whether userspace decides to freeze the tasks or not).  I think it's better that we /try/ to do the eviction and notify them if userspace should be changed.

Hmm, the logic is kind of inconsistent now.

For dGPUs, evict is required for s0ix, s3, s4 and attempted twice. For APUs, evict is required for s4, but attempted only once.

Thanks,
Lijo

Quoted 25 lines
Quoted 24 lines

Thanks,
Lijo

Quoted 20 lines

+    case PM_SUSPEND_PREPARE:
+        r = amdgpu_device_evict_resources(adev);
+        adev->in_s4 = false;
+        /*
+         * This is considered non-fatal at thie time because +         * amdgpu_device_prepare() will also evict resources. +         * See https://gitlab.freedesktop.org/drm/amd/-/issues/3781
+         */
+        if (r)
+            drm_warn(adev_to_drm(adev), "Failed to evict resources, freeze active processes if problems occur\n");
+        break;
+    }

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +4 -0
+
+    return NOTIFY_DONE;
+}
+

  /**
   * amdgpu_device_prepare - prepare for device suspend *

#5 lore raw

Re: [PATCH v4 2/2] drm/amd: Add Suspend/Hibernate notification callback support

Quoted 12 lines
Quoted 11 lines
Quoted 4 lines

Based on https://gitlab.freedesktop.org/drm/amd/-/issues/3781

What if this callback takes care only of suspend case and leaves the hibernate case to dpm_prepare callback?

Then hibernate would fail under memory pressure.

My take is this failure with hibernate is a userspace problem (whether userspace decides to freeze the tasks or not).  I think it's better that we /try/ to do the eviction and notify them if userspace should be changed.

Quoted 4 lines

Hmm, the logic is kind of inconsistent now.

For dGPUs, evict is required for s0ix, s3, s4 and attempted twice. For APUs, evict is required for s4, but attempted only once.

Well the logic was inconsistent before already.

But FWIW it will still be attempted a second time for APUs. The call sequence is:

1) PM notifier
amdgpu_device_pm_notifier()
->amdgpu_device_evict_resources()

Eviction for dGPU and APU in S4.

2) Prepare callback
amdgpu_pmops_prepare()
->amdgpu_device_prepare()
->->amdgpu_device_evict_resources()

Eviction for dGPU only.

3) Suspend callback
amdgpu_pmops_freeze()
->amdgpu_device_suspend()
->->amdgpu_device_evict_resources()

Eviction for dGPU and APU in S4.

So yes it's incongruent that prepare() doesn't include APU S4, but the problem is you won't know at prepare() time whether it's S4 AFAICT.

Or is there way to tell at prepare() for S4?

Quoted 30 lines

Thanks,
Lijo

Quoted 25 lines
Quoted 24 lines

Thanks,
Lijo

Quoted 20 lines

+    case PM_SUSPEND_PREPARE:
+        r = amdgpu_device_evict_resources(adev);
+        adev->in_s4 = false;
+        /*
+         * This is considered non-fatal at thie time because +         * amdgpu_device_prepare() will also evict resources. +         * See https://gitlab.freedesktop.org/drm/amd/-/issues/3781
+         */
+        if (r)
+            drm_warn(adev_to_drm(adev), "Failed to evict resources, freeze active processes if problems occur\n");
+        break;
+    }

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +4 -0
+
+    return NOTIFY_DONE;
+}
+
   /**
    * amdgpu_device_prepare - prepare for device suspend
    *
#6 lore raw

Re: [PATCH v4 2/2] drm/amd: Add Suspend/Hibernate notification callback support

Quoted 41 lines · Mario Limonciello
Quoted 13 lines
Quoted 5 lines
Quoted 4 lines

Based on https://gitlab.freedesktop.org/drm/amd/-/issues/3781

What if this callback takes care only of suspend case and leaves the hibernate case to dpm_prepare callback?

Quoted 7 lines

Then hibernate would fail under memory pressure.

My take is this failure with hibernate is a userspace problem (whether userspace decides to freeze the tasks or not).  I think it's better that we /try/ to do the eviction and notify them if userspace should be changed.

Quoted 4 lines

Hmm, the logic is kind of inconsistent now.

For dGPUs, evict is required for s0ix, s3, s4 and attempted twice. For APUs, evict is required for s4, but attempted only once.

Well the logic was inconsistent before already.

But FWIW it will still be attempted a second time for APUs. The call sequence is:

1) PM notifier
amdgpu_device_pm_notifier()
->amdgpu_device_evict_resources()

Eviction for dGPU and APU in S4.

2) Prepare callback
amdgpu_pmops_prepare()
->amdgpu_device_prepare()
->->amdgpu_device_evict_resources()

Eviction for dGPU only.

3) Suspend callback
amdgpu_pmops_freeze()
->amdgpu_device_suspend()
->->amdgpu_device_evict_resources()

Quoted 7 lines

Eviction for dGPU and APU in S4.

So yes it's incongruent that prepare() doesn't include APU S4, but the problem is you won't know at prepare() time whether it's S4 AFAICT.

Or is there way to tell at prepare() for S4?

Now this notifier comes in before that and tells that right?

BTW, I didn't realize that there is a max of 3 attempts now (didn't notice it being called in suspend also).

Thanks,
Lijo

Quoted 31 lines
Quoted 30 lines

Thanks,
Lijo

Quoted 25 lines
Quoted 24 lines

Thanks,
Lijo

Quoted 20 lines

+    case PM_SUSPEND_PREPARE:
+        r = amdgpu_device_evict_resources(adev);
+        adev->in_s4 = false;
+        /*
+         * This is considered non-fatal at thie time because +         * amdgpu_device_prepare() will also evict resources. +         * See https://gitlab.freedesktop.org/drm/amd/-/issues/3781
+         */
+        if (r)
+            drm_warn(adev_to_drm(adev), "Failed to evict resources, freeze active processes if problems occur\n");
+        break;
+    }

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +4 -0
+
+    return NOTIFY_DONE;
+}
+

   /**
    * amdgpu_device_prepare - prepare for device suspend *

#7 lore raw

Re: [PATCH v4 2/2] drm/amd: Add Suspend/Hibernate notification callback support

Quoted 44 lines · Lijo
Quoted 41 lines · Mario Limonciello
Quoted 13 lines
Quoted 5 lines
Quoted 4 lines

Based on https://gitlab.freedesktop.org/drm/amd/-/issues/3781

What if this callback takes care only of suspend case and leaves the hibernate case to dpm_prepare callback?

Quoted 7 lines

Then hibernate would fail under memory pressure.

My take is this failure with hibernate is a userspace problem (whether userspace decides to freeze the tasks or not).  I think it's better that we /try/ to do the eviction and notify them if userspace should be changed.

Quoted 4 lines

Hmm, the logic is kind of inconsistent now.

For dGPUs, evict is required for s0ix, s3, s4 and attempted twice. For APUs, evict is required for s4, but attempted only once.

Well the logic was inconsistent before already.

But FWIW it will still be attempted a second time for APUs. The call sequence is:

1) PM notifier
amdgpu_device_pm_notifier()
->amdgpu_device_evict_resources()

Eviction for dGPU and APU in S4.

2) Prepare callback
amdgpu_pmops_prepare()
->amdgpu_device_prepare()
->->amdgpu_device_evict_resources()

Eviction for dGPU only.

3) Suspend callback
amdgpu_pmops_freeze()
->amdgpu_device_suspend()
->->amdgpu_device_evict_resources()

Quoted 9 lines
Quoted 7 lines

Eviction for dGPU and APU in S4.

So yes it's incongruent that prepare() doesn't include APU S4, but the problem is you won't know at prepare() time whether it's S4 AFAICT.

Or is there way to tell at prepare() for S4?

Now this notifier comes in before that and tells that right?

Oh yeah, it does!

In that case we can set adev-in_s4 during the PM notifier and keep it set unless there is a failure during prepare().

Quoted 2 lines

BTW, I didn't realize that there is a max of 3 attempts now (didn't notice it being called in suspend also).

Yeah, there are other reasons suspend callback gets called so it's better to keep that sequence identical.

Quoted 36 lines

Thanks,
Lijo

Quoted 31 lines
Quoted 30 lines

Thanks,
Lijo

Quoted 25 lines
Quoted 24 lines

Thanks,
Lijo

Quoted 20 lines

+    case PM_SUSPEND_PREPARE:
+        r = amdgpu_device_evict_resources(adev);
+        adev->in_s4 = false;
+        /*
+         * This is considered non-fatal at thie time because +         * amdgpu_device_prepare() will also evict resources. +         * See https://gitlab.freedesktop.org/drm/amd/-/issues/3781
+         */
+        if (r)
+            drm_warn(adev_to_drm(adev), "Failed to evict resources, freeze active processes if problems occur\n");
+        break;
+    }

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +4 -0
+
+    return NOTIFY_DONE;
+}
+
    /**
     * amdgpu_device_prepare - prepare for device suspend
     *