[RFC 0/2] Evict VRAM after processes are frozen

[RFC 0/2] Evict VRAM after processes are frozen

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

As part of the suspend and hibernate sequences devices supported
by amdgpu will have VRAM evicted.  This has been littered with problems
in the past, and now it is called strategically at multiple times
during power management sequences.

Most recently there were a number of issues reported that were root caused
to physical memory pressure at suspend time where VRAM couldn't be evicted.
A new call was introduced using a PM notifier before swap was turned off
so that even if there are physical memory pressures VRAM could still be
evicted into swap.

This helped, but users using older systemd noted that this could lead to a
deadlock.  This is because user processes aren't frozen by older versions
of systemd and thus if a game is running it could potentially be creating
BOs during the eviction thus preventing eviction from ever completing.

This isn't an issue with newer systemd because it uses cgroup freezing
to freeze user processes. Nonetheless this isn't a good behavior to rely
upon for userspace, so this series aims to solve the problem in another way.

Introduce new PM notifier calls after "prepare" notifier is done AND after
tasks have been frozen.  amdgpu listens to these PM notifier calls and
will run the first eviction at this time.

Mario Limonciello (2):
  PM: Add suspend and hibernate notifications for after freeze
  drm/amd: Use suspend and hibernate post freeze notifications

 Documentation/driver-api/pm/notifiers.rst  | 19 ++++++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 ++--
 include/linux/suspend.h                    | 14 ++++++++------
 kernel/power/hibernate.c                   |  9 +++++++--
 kernel/power/suspend.c                     | 13 +++++++++----
 5 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.43.0

[RFC 1/2] PM: Add suspend and hibernate notifications for after freeze

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Suspend and hibernate notifications are available specifically when
the sequence starts and finishes.  However there are no notifications
during the process when tasks have been frozen.

Introduce two new events `PM_SUSPEND_POST_FREEZE` and
`PM_HIBERNATE_POST_FREEZE` that drivers can subscribe to and take
different actions specifically knowing userspace is frozen.

Suggested-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 Documentation/driver-api/pm/notifiers.rst | 19 ++++++++++++++++---
 include/linux/suspend.h                   | 14 ++++++++------
 kernel/power/hibernate.c                  |  9 +++++++--
 kernel/power/suspend.c                    | 13 +++++++++----
 4 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/Documentation/driver-api/pm/notifiers.rst b/Documentation/driver-api/pm/notifiers.rst
index 186435c43b77e..6a1912fbee214 100644
--- a/Documentation/driver-api/pm/notifiers.rst
+++ b/Documentation/driver-api/pm/notifiers.rst
@@ -32,6 +32,18 @@ will be called upon the following events by the PM core:
 	additional work is done between the notifiers and the invocation of PM
 	callbacks for the "freeze" transition.
 
+``PM_HIBERNATION_POST_FREEZE``
+	The system is going to hibernate and tasks have just been frozen.
+
+``PM_SUSPEND_PREPARE``
+	The system is going to suspend, tasks will be frozen immediately.  This
+	is different from ``PM_HIBERNATION_PREPARE`` above, because in this case
+	additional work is done between the notifiers and the invocation of PM
+	callbacks for the "freeze" transition.
+
+``PM_SUSPEND_POST_FREEZE``
+	The system is going to suspend and tasks have just been frozen.
+
 ``PM_POST_HIBERNATION``
 	The system memory state has been restored from a hibernation image or an
 	error occurred during hibernation.  Device restore callbacks have been
@@ -54,9 +66,10 @@ will be called upon the following events by the PM core:
 	resume callbacks have been executed and tasks have been thawed.
 
 It is generally assumed that whatever the notifiers do for
-``PM_HIBERNATION_PREPARE``, should be undone for ``PM_POST_HIBERNATION``.
-Analogously, operations carried out for ``PM_SUSPEND_PREPARE`` should be
-reversed for ``PM_POST_SUSPEND``.
+``PM_HIBERNATION_PREPARE`` and ``PM_HIBERNATION_POST_FREEZE``, should be undone
+for ``PM_POST_HIBERNATION``.
+Analogously, operations carried out for ``PM_SUSPEND_PREPARE`` and ``PM_SUSPEND_POST_FREEZE``
+should be reversed for ``PM_POST_SUSPEND``.
 
 Moreover, if one of the notifiers fails for the ``PM_HIBERNATION_PREPARE`` or
 ``PM_SUSPEND_PREPARE`` event, the notifiers that have already succeeded for that
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index da6ebca3ff774..704e6579b0df6 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -422,12 +422,14 @@ static inline int is_hibernate_resume_dev(dev_t dev) { return 0; }
 #endif
 
 /* Hibernation and suspend events */
-#define PM_HIBERNATION_PREPARE	0x0001 /* Going to hibernate */
-#define PM_POST_HIBERNATION	0x0002 /* Hibernation finished */
-#define PM_SUSPEND_PREPARE	0x0003 /* Going to suspend the system */
-#define PM_POST_SUSPEND		0x0004 /* Suspend finished */
-#define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
-#define PM_POST_RESTORE		0x0006 /* Restore failed */
+#define PM_HIBERNATION_PREPARE		0x0001 /* Going to hibernate */
+#define PM_HIBERNATION_POST_FREEZE	0x0002 /* Prepared for hibernation and tasks have been frozen */
+#define PM_POST_HIBERNATION		0x0003 /* Hibernation finished */
+#define PM_SUSPEND_PREPARE		0x0004 /* Going to suspend the system */
+#define PM_SUSPEND_POST_FREEZE		0x0005 /* Prepared and tasks have been frozen */
+#define PM_POST_SUSPEND			0x0006 /* Suspend finished */
+#define PM_RESTORE_PREPARE		0x0007 /* Going to restore a saved image */
+#define PM_POST_RESTORE			0x0008 /* Restore failed */
 
 extern struct mutex system_transition_mutex;
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index f0db9d1896e80..f896056ad2e5d 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -783,11 +783,15 @@ int hibernate(void)
 	if (error)
 		goto Exit;
 
+	error = pm_notifier_call_chain_robust(PM_HIBERNATION_POST_FREEZE, PM_POST_HIBERNATION);
+	if (error)
+		goto Thaw;
+
 	lock_device_hotplug();
 	/* Allocate memory management structures */
 	error = create_basic_memory_bitmaps();
 	if (error)
-		goto Thaw;
+		goto Unlock_hotplug;
 
 	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
 	if (error || freezer_test_done)
@@ -833,8 +837,9 @@ int hibernate(void)
 
  Free_bitmaps:
 	free_basic_memory_bitmaps();
- Thaw:
+ Unlock_hotplug:
 	unlock_device_hotplug();
+ Thaw:
 	if (snapshot_test) {
 		pm_pr_dbg("Checking hibernation image\n");
 		error = swsusp_check(false);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 8eaec4ab121d4..bc6654e8cdc80 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -377,13 +377,18 @@ static int suspend_prepare(suspend_state_t state)
 	trace_suspend_resume(TPS("freeze_processes"), 0, true);
 	error = suspend_freeze_processes();
 	trace_suspend_resume(TPS("freeze_processes"), 0, false);
-	if (!error)
-		return 0;
+	if (error)
+		goto Restore;
+	error = pm_notifier_call_chain_robust(PM_SUSPEND_POST_FREEZE, PM_POST_SUSPEND);
+	if (error)
+		goto Thaw;
 
-	dpm_save_failed_step(SUSPEND_FREEZE);
-	pm_notifier_call_chain(PM_POST_SUSPEND);
+	return 0;
+ Thaw:
+	suspend_thaw_processes();
  Restore:
 	pm_restore_console();
+	dpm_save_failed_step(SUSPEND_FREEZE);
 	return error;
 }
 
-- 
2.43.0

[RFC 2/2] drm/amd: Use suspend and hibernate post freeze notifications

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

commit 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification
callback support") introduced a VRAM eviction earlier in the PM
sequences when swap was still available for evicting to. This helped
to fix a number of memory pressure related bugs but also exposed a
new one.

If a userspace process is actively using the GPU when suspend starts
then a deadlock could occur.

Instead of going off the prepare notifier, use the PM notifiers that
occur after processes have been frozen to do evictions.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4178
Fixes: 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification callback support")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 7f354cd532dc1..cad311b9fd834 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4917,10 +4917,10 @@ static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mo
 	int r;
 
 	switch (mode) {
-	case PM_HIBERNATION_PREPARE:
+	case PM_HIBERNATION_POST_FREEZE:
 		adev->in_s4 = true;
 		fallthrough;
-	case PM_SUSPEND_PREPARE:
+	case PM_SUSPEND_POST_FREEZE:
 		r = amdgpu_device_evict_resources(adev);
 		/*
 		 * This is considered non-fatal at this time because
-- 
2.43.0

Re: [RFC 0/2] Evict VRAM after processes are frozen

[Christian König]

On 5/1/25 23:17, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> As part of the suspend and hibernate sequences devices supported
> by amdgpu will have VRAM evicted.  This has been littered with problems
> in the past, and now it is called strategically at multiple times
> during power management sequences.
> 
> Most recently there were a number of issues reported that were root caused
> to physical memory pressure at suspend time where VRAM couldn't be evicted.
> A new call was introduced using a PM notifier before swap was turned off
> so that even if there are physical memory pressures VRAM could still be
> evicted into swap.
> 
> This helped, but users using older systemd noted that this could lead to a
> deadlock.  This is because user processes aren't frozen by older versions
> of systemd and thus if a game is running it could potentially be creating
> BOs during the eviction thus preventing eviction from ever completing.
> 
> This isn't an issue with newer systemd because it uses cgroup freezing
> to freeze user processes. Nonetheless this isn't a good behavior to rely
> upon for userspace, so this series aims to solve the problem in another way.
> 
> Introduce new PM notifier calls after "prepare" notifier is done AND after
> tasks have been frozen.  amdgpu listens to these PM notifier calls and
> will run the first eviction at this time.

Sounds totally sane to me, but I'm really not deep enough in the PM code to fully judge the implementation.

Feel free to add Acked-by: Christian König <christian.koenig@amd.com> to the patches.

Regards,
Christian.

> 
> Mario Limonciello (2):
>   PM: Add suspend and hibernate notifications for after freeze
>   drm/amd: Use suspend and hibernate post freeze notifications
> 
>  Documentation/driver-api/pm/notifiers.rst  | 19 ++++++++++++++++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 ++--
>  include/linux/suspend.h                    | 14 ++++++++------
>  kernel/power/hibernate.c                   |  9 +++++++--
>  kernel/power/suspend.c                     | 13 +++++++++----
>  5 files changed, 42 insertions(+), 17 deletions(-)
> 

Re: [RFC 1/2] PM: Add suspend and hibernate notifications for after freeze

[Mario Limonciello]

On 5/1/2025 4:17 PM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Suspend and hibernate notifications are available specifically when
> the sequence starts and finishes.  However there are no notifications
> during the process when tasks have been frozen.
> 
> Introduce two new events `PM_SUSPEND_POST_FREEZE` and
> `PM_HIBERNATE_POST_FREEZE` that drivers can subscribe to and take
> different actions specifically knowing userspace is frozen.
> 
> Suggested-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   Documentation/driver-api/pm/notifiers.rst | 19 ++++++++++++++++---
>   include/linux/suspend.h                   | 14 ++++++++------
>   kernel/power/hibernate.c                  |  9 +++++++--
>   kernel/power/suspend.c                    | 13 +++++++++----
>   4 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/driver-api/pm/notifiers.rst b/Documentation/driver-api/pm/notifiers.rst
> index 186435c43b77e..6a1912fbee214 100644
> --- a/Documentation/driver-api/pm/notifiers.rst
> +++ b/Documentation/driver-api/pm/notifiers.rst
> @@ -32,6 +32,18 @@ will be called upon the following events by the PM core:
>   	additional work is done between the notifiers and the invocation of PM
>   	callbacks for the "freeze" transition.
>   
> +``PM_HIBERNATION_POST_FREEZE``
> +	The system is going to hibernate and tasks have just been frozen.
> +
> +``PM_SUSPEND_PREPARE``
> +	The system is going to suspend, tasks will be frozen immediately.  This
> +	is different from ``PM_HIBERNATION_PREPARE`` above, because in this case
> +	additional work is done between the notifiers and the invocation of PM
> +	callbacks for the "freeze" transition.

I accidentally created this section ``PM_SUSPEND_PREPARE`` (there 
already is one for PM_SUSPEND_PREPARE that is totally fine).

If conceptually the rest of the patch makes sense I will drop this section.

> +
> +``PM_SUSPEND_POST_FREEZE``
> +	The system is going to suspend and tasks have just been frozen.

Looking again, I think this is better put between (the correct) 
PM_SUSPEND_PREPARE and PM_POST_SUSPEND.

If conceptually the rest of the patch makes sense I will move it to that 
section.

> +
>   ``PM_POST_HIBERNATION``
>   	The system memory state has been restored from a hibernation image or an
>   	error occurred during hibernation.  Device restore callbacks have been
> @@ -54,9 +66,10 @@ will be called upon the following events by the PM core:
>   	resume callbacks have been executed and tasks have been thawed.
>   
>   It is generally assumed that whatever the notifiers do for
> -``PM_HIBERNATION_PREPARE``, should be undone for ``PM_POST_HIBERNATION``.
> -Analogously, operations carried out for ``PM_SUSPEND_PREPARE`` should be
> -reversed for ``PM_POST_SUSPEND``.
> +``PM_HIBERNATION_PREPARE`` and ``PM_HIBERNATION_POST_FREEZE``, should be undone
> +for ``PM_POST_HIBERNATION``.
> +Analogously, operations carried out for ``PM_SUSPEND_PREPARE`` and ``PM_SUSPEND_POST_FREEZE``
> +should be reversed for ``PM_POST_SUSPEND``.
>   
>   Moreover, if one of the notifiers fails for the ``PM_HIBERNATION_PREPARE`` or
>   ``PM_SUSPEND_PREPARE`` event, the notifiers that have already succeeded for that
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index da6ebca3ff774..704e6579b0df6 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -422,12 +422,14 @@ static inline int is_hibernate_resume_dev(dev_t dev) { return 0; }
>   #endif
>   
>   /* Hibernation and suspend events */
> -#define PM_HIBERNATION_PREPARE	0x0001 /* Going to hibernate */
> -#define PM_POST_HIBERNATION	0x0002 /* Hibernation finished */
> -#define PM_SUSPEND_PREPARE	0x0003 /* Going to suspend the system */
> -#define PM_POST_SUSPEND		0x0004 /* Suspend finished */
> -#define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
> -#define PM_POST_RESTORE		0x0006 /* Restore failed */
> +#define PM_HIBERNATION_PREPARE		0x0001 /* Going to hibernate */
> +#define PM_HIBERNATION_POST_FREEZE	0x0002 /* Prepared for hibernation and tasks have been frozen */
> +#define PM_POST_HIBERNATION		0x0003 /* Hibernation finished */
> +#define PM_SUSPEND_PREPARE		0x0004 /* Going to suspend the system */
> +#define PM_SUSPEND_POST_FREEZE		0x0005 /* Prepared and tasks have been frozen */
> +#define PM_POST_SUSPEND			0x0006 /* Suspend finished */
> +#define PM_RESTORE_PREPARE		0x0007 /* Going to restore a saved image */
> +#define PM_POST_RESTORE			0x0008 /* Restore failed */
>   
>   extern struct mutex system_transition_mutex;
>   
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index f0db9d1896e80..f896056ad2e5d 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -783,11 +783,15 @@ int hibernate(void)
>   	if (error)
>   		goto Exit;
>   
> +	error = pm_notifier_call_chain_robust(PM_HIBERNATION_POST_FREEZE, PM_POST_HIBERNATION);
> +	if (error)
> +		goto Thaw;
> +
>   	lock_device_hotplug();
>   	/* Allocate memory management structures */
>   	error = create_basic_memory_bitmaps();
>   	if (error)
> -		goto Thaw;
> +		goto Unlock_hotplug;
>   
>   	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
>   	if (error || freezer_test_done)
> @@ -833,8 +837,9 @@ int hibernate(void)
>   
>    Free_bitmaps:
>   	free_basic_memory_bitmaps();
> - Thaw:
> + Unlock_hotplug:
>   	unlock_device_hotplug();
> + Thaw:
>   	if (snapshot_test) {
>   		pm_pr_dbg("Checking hibernation image\n");
>   		error = swsusp_check(false);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8eaec4ab121d4..bc6654e8cdc80 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -377,13 +377,18 @@ static int suspend_prepare(suspend_state_t state)
>   	trace_suspend_resume(TPS("freeze_processes"), 0, true);
>   	error = suspend_freeze_processes();
>   	trace_suspend_resume(TPS("freeze_processes"), 0, false);
> -	if (!error)
> -		return 0;
> +	if (error)
> +		goto Restore;
> +	error = pm_notifier_call_chain_robust(PM_SUSPEND_POST_FREEZE, PM_POST_SUSPEND);
> +	if (error)
> +		goto Thaw;
>   
> -	dpm_save_failed_step(SUSPEND_FREEZE);
> -	pm_notifier_call_chain(PM_POST_SUSPEND);
> +	return 0;
> + Thaw:
> +	suspend_thaw_processes();
>    Restore:
>   	pm_restore_console();
> +	dpm_save_failed_step(SUSPEND_FREEZE);
>   	return error;
>   }
>   

Re: [RFC 2/2] drm/amd: Use suspend and hibernate post freeze notifications

[Rafael J. Wysocki]

On Thu, May 1, 2025 at 11:17 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> commit 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification
> callback support") introduced a VRAM eviction earlier in the PM
> sequences when swap was still available for evicting to. This helped
> to fix a number of memory pressure related bugs but also exposed a
> new one.
>
> If a userspace process is actively using the GPU when suspend starts
> then a deadlock could occur.
>
> Instead of going off the prepare notifier, use the PM notifiers that
> occur after processes have been frozen to do evictions.
>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4178
> Fixes: 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification callback support")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 7f354cd532dc1..cad311b9fd834 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4917,10 +4917,10 @@ static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mo
>         int r;
>
>         switch (mode) {
> -       case PM_HIBERNATION_PREPARE:
> +       case PM_HIBERNATION_POST_FREEZE:
>                 adev->in_s4 = true;
>                 fallthrough;
> -       case PM_SUSPEND_PREPARE:
> +       case PM_SUSPEND_POST_FREEZE:
>                 r = amdgpu_device_evict_resources(adev);
>                 /*
>                  * This is considered non-fatal at this time because
> --

Why do you need a notifier for this?

It looks like this could be done from amdgpu_device_prepare(), but if
there is a reason why it cannot be done from there, it should be
mentioned in the changelog.

Re: [RFC 2/2] drm/amd: Use suspend and hibernate post freeze notifications

[Mario Limonciello]

On 5/7/2025 2:14 PM, Rafael J. Wysocki wrote:
> On Thu, May 1, 2025 at 11:17 PM Mario Limonciello <superm1@kernel.org> wrote:
>>
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> commit 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification
>> callback support") introduced a VRAM eviction earlier in the PM
>> sequences when swap was still available for evicting to. This helped
>> to fix a number of memory pressure related bugs but also exposed a
>> new one.
>>
>> If a userspace process is actively using the GPU when suspend starts
>> then a deadlock could occur.
>>
>> Instead of going off the prepare notifier, use the PM notifiers that
>> occur after processes have been frozen to do evictions.
>>
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4178
>> Fixes: 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification callback support")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 7f354cd532dc1..cad311b9fd834 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -4917,10 +4917,10 @@ static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mo
>>          int r;
>>
>>          switch (mode) {
>> -       case PM_HIBERNATION_PREPARE:
>> +       case PM_HIBERNATION_POST_FREEZE:
>>                  adev->in_s4 = true;
>>                  fallthrough;
>> -       case PM_SUSPEND_PREPARE:
>> +       case PM_SUSPEND_POST_FREEZE:
>>                  r = amdgpu_device_evict_resources(adev);
>>                  /*
>>                   * This is considered non-fatal at this time because
>> --
> 
> Why do you need a notifier for this?
> 
> It looks like this could be done from amdgpu_device_prepare(), but if
> there is a reason why it cannot be done from there, it should be
> mentioned in the changelog.

It's actually done in amdgpu_device_prepare() "as well" already, but the 
reason that it's being done earlier is because swap still needs to be 
available, especially with heavy memory fragmentation.

I'll add more detail about this to the commit for the next spin if 
you're relatively happy with the new notifier from the first patch.

Re: [RFC 2/2] drm/amd: Use suspend and hibernate post freeze notifications

[Rafael J. Wysocki]

On Wed, May 7, 2025 at 9:17 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 5/7/2025 2:14 PM, Rafael J. Wysocki wrote:
> > On Thu, May 1, 2025 at 11:17 PM Mario Limonciello <superm1@kernel.org> wrote:
> >>
> >> From: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> commit 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification
> >> callback support") introduced a VRAM eviction earlier in the PM
> >> sequences when swap was still available for evicting to. This helped
> >> to fix a number of memory pressure related bugs but also exposed a
> >> new one.
> >>
> >> If a userspace process is actively using the GPU when suspend starts
> >> then a deadlock could occur.
> >>
> >> Instead of going off the prepare notifier, use the PM notifiers that
> >> occur after processes have been frozen to do evictions.
> >>
> >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4178
> >> Fixes: 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification callback support")
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 7f354cd532dc1..cad311b9fd834 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -4917,10 +4917,10 @@ static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mo
> >>          int r;
> >>
> >>          switch (mode) {
> >> -       case PM_HIBERNATION_PREPARE:
> >> +       case PM_HIBERNATION_POST_FREEZE:
> >>                  adev->in_s4 = true;
> >>                  fallthrough;
> >> -       case PM_SUSPEND_PREPARE:
> >> +       case PM_SUSPEND_POST_FREEZE:
> >>                  r = amdgpu_device_evict_resources(adev);
> >>                  /*
> >>                   * This is considered non-fatal at this time because
> >> --
> >
> > Why do you need a notifier for this?
> >
> > It looks like this could be done from amdgpu_device_prepare(), but if
> > there is a reason why it cannot be done from there, it should be
> > mentioned in the changelog.
>
> It's actually done in amdgpu_device_prepare() "as well" already, but the
> reason that it's being done earlier is because swap still needs to be
> available, especially with heavy memory fragmentation.

Swap should be still available when amdgpu_device_prepare() runs.

> I'll add more detail about this to the commit for the next spin if
> you're relatively happy with the new notifier from the first patch.

I need to have a look at it, but adding it for just one user seems a
bit over the top.  I'd prefer to avoid doing this.

Re: [RFC 2/2] drm/amd: Use suspend and hibernate post freeze notifications

[Alex Deucher]

On Wed, May 7, 2025 at 3:39 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, May 7, 2025 at 9:17 PM Mario Limonciello <superm1@kernel.org> wrote:
> >
> > On 5/7/2025 2:14 PM, Rafael J. Wysocki wrote:
> > > On Thu, May 1, 2025 at 11:17 PM Mario Limonciello <superm1@kernel.org> wrote:
> > >>
> > >> From: Mario Limonciello <mario.limonciello@amd.com>
> > >>
> > >> commit 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification
> > >> callback support") introduced a VRAM eviction earlier in the PM
> > >> sequences when swap was still available for evicting to. This helped
> > >> to fix a number of memory pressure related bugs but also exposed a
> > >> new one.
> > >>
> > >> If a userspace process is actively using the GPU when suspend starts
> > >> then a deadlock could occur.
> > >>
> > >> Instead of going off the prepare notifier, use the PM notifiers that
> > >> occur after processes have been frozen to do evictions.
> > >>
> > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4178
> > >> Fixes: 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification callback support")
> > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > >> ---
> > >>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
> > >>   1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > >> index 7f354cd532dc1..cad311b9fd834 100644
> > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > >> @@ -4917,10 +4917,10 @@ static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mo
> > >>          int r;
> > >>
> > >>          switch (mode) {
> > >> -       case PM_HIBERNATION_PREPARE:
> > >> +       case PM_HIBERNATION_POST_FREEZE:
> > >>                  adev->in_s4 = true;
> > >>                  fallthrough;
> > >> -       case PM_SUSPEND_PREPARE:
> > >> +       case PM_SUSPEND_POST_FREEZE:
> > >>                  r = amdgpu_device_evict_resources(adev);
> > >>                  /*
> > >>                   * This is considered non-fatal at this time because
> > >> --
> > >
> > > Why do you need a notifier for this?
> > >
> > > It looks like this could be done from amdgpu_device_prepare(), but if
> > > there is a reason why it cannot be done from there, it should be
> > > mentioned in the changelog.
> >
> > It's actually done in amdgpu_device_prepare() "as well" already, but the
> > reason that it's being done earlier is because swap still needs to be
> > available, especially with heavy memory fragmentation.
>
> Swap should be still available when amdgpu_device_prepare() runs.

We need to do this after processes are frozen, otherwise applications
keep running and keep allocating VRAM so by the time processes are
frozen, swap is disabled and we don't have anywhere to store the
contents of VRAM if memory is full or fragmented.

Alex

>
> > I'll add more detail about this to the commit for the next spin if
> > you're relatively happy with the new notifier from the first patch.
>
> I need to have a look at it, but adding it for just one user seems a
> bit over the top.  I'd prefer to avoid doing this.

Re: [RFC 2/2] drm/amd: Use suspend and hibernate post freeze notifications

[Mario Limonciello]

On 5/7/2025 2:39 PM, Rafael J. Wysocki wrote:
> On Wed, May 7, 2025 at 9:17 PM Mario Limonciello <superm1@kernel.org> wrote:
>>
>> On 5/7/2025 2:14 PM, Rafael J. Wysocki wrote:
>>> On Thu, May 1, 2025 at 11:17 PM Mario Limonciello <superm1@kernel.org> wrote:
>>>>
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> commit 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification
>>>> callback support") introduced a VRAM eviction earlier in the PM
>>>> sequences when swap was still available for evicting to. This helped
>>>> to fix a number of memory pressure related bugs but also exposed a
>>>> new one.
>>>>
>>>> If a userspace process is actively using the GPU when suspend starts
>>>> then a deadlock could occur.
>>>>
>>>> Instead of going off the prepare notifier, use the PM notifiers that
>>>> occur after processes have been frozen to do evictions.
>>>>
>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4178
>>>> Fixes: 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification callback support")
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index 7f354cd532dc1..cad311b9fd834 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -4917,10 +4917,10 @@ static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mo
>>>>           int r;
>>>>
>>>>           switch (mode) {
>>>> -       case PM_HIBERNATION_PREPARE:
>>>> +       case PM_HIBERNATION_POST_FREEZE:
>>>>                   adev->in_s4 = true;
>>>>                   fallthrough;
>>>> -       case PM_SUSPEND_PREPARE:
>>>> +       case PM_SUSPEND_POST_FREEZE:
>>>>                   r = amdgpu_device_evict_resources(adev);
>>>>                   /*
>>>>                    * This is considered non-fatal at this time because
>>>> --
>>>
>>> Why do you need a notifier for this?
>>>
>>> It looks like this could be done from amdgpu_device_prepare(), but if
>>> there is a reason why it cannot be done from there, it should be
>>> mentioned in the changelog.
>>
>> It's actually done in amdgpu_device_prepare() "as well" already, but the
>> reason that it's being done earlier is because swap still needs to be
>> available, especially with heavy memory fragmentation.
> 
> Swap should be still available when amdgpu_device_prepare() runs.

No; it's not.  The basic call trace (for suspend) looks like this:

enter_state(state) {
     suspend_prepare(state);
     ...
     pm_restrict_gfp_mask();  // disable swap
     suspend_devices_and_enter(state) → dpm_suspend_start() {
         dpm_prepare() {
             amdgpu_pmops_prepare();
         }
         dpm_suspend() {
             amdgpu_pmops_suspend();
         }
     }
}

If the intention was for it to be available, it would be better to move 
the pm_restrict_gfp_mask() call "into" suspend_devices_and_enter() 
between dpm_prepare() and dpm_suspend() calls.

> 
>> I'll add more detail about this to the commit for the next spin if
>> you're relatively happy with the new notifier from the first patch.
> 
> I need to have a look at it, but adding it for just one user seems a
> bit over the top.  I'd prefer to avoid doing this.

Re: [RFC 2/2] drm/amd: Use suspend and hibernate post freeze notifications

[Rafael J. Wysocki]

On Wed, May 7, 2025 at 9:45 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 5/7/2025 2:39 PM, Rafael J. Wysocki wrote:
> > On Wed, May 7, 2025 at 9:17 PM Mario Limonciello <superm1@kernel.org> wrote:
> >>
> >> On 5/7/2025 2:14 PM, Rafael J. Wysocki wrote:
> >>> On Thu, May 1, 2025 at 11:17 PM Mario Limonciello <superm1@kernel.org> wrote:
> >>>>
> >>>> From: Mario Limonciello <mario.limonciello@amd.com>
> >>>>
> >>>> commit 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification
> >>>> callback support") introduced a VRAM eviction earlier in the PM
> >>>> sequences when swap was still available for evicting to. This helped
> >>>> to fix a number of memory pressure related bugs but also exposed a
> >>>> new one.
> >>>>
> >>>> If a userspace process is actively using the GPU when suspend starts
> >>>> then a deadlock could occur.
> >>>>
> >>>> Instead of going off the prepare notifier, use the PM notifiers that
> >>>> occur after processes have been frozen to do evictions.
> >>>>
> >>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4178
> >>>> Fixes: 2965e6355dcd ("drm/amd: Add Suspend/Hibernate notification callback support")
> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>>> ---
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>> index 7f354cd532dc1..cad311b9fd834 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>> @@ -4917,10 +4917,10 @@ static int amdgpu_device_pm_notifier(struct notifier_block *nb, unsigned long mo
> >>>>           int r;
> >>>>
> >>>>           switch (mode) {
> >>>> -       case PM_HIBERNATION_PREPARE:
> >>>> +       case PM_HIBERNATION_POST_FREEZE:
> >>>>                   adev->in_s4 = true;
> >>>>                   fallthrough;
> >>>> -       case PM_SUSPEND_PREPARE:
> >>>> +       case PM_SUSPEND_POST_FREEZE:
> >>>>                   r = amdgpu_device_evict_resources(adev);
> >>>>                   /*
> >>>>                    * This is considered non-fatal at this time because
> >>>> --
> >>>
> >>> Why do you need a notifier for this?
> >>>
> >>> It looks like this could be done from amdgpu_device_prepare(), but if
> >>> there is a reason why it cannot be done from there, it should be
> >>> mentioned in the changelog.
> >>
> >> It's actually done in amdgpu_device_prepare() "as well" already, but the
> >> reason that it's being done earlier is because swap still needs to be
> >> available, especially with heavy memory fragmentation.
> >
> > Swap should be still available when amdgpu_device_prepare() runs.
>
> No; it's not.  The basic call trace (for suspend) looks like this:
>
> enter_state(state) {
>      suspend_prepare(state);
>      ...
>      pm_restrict_gfp_mask();  // disable swap

Ah, OK

>      suspend_devices_and_enter(state) → dpm_suspend_start() {
>          dpm_prepare() {
>              amdgpu_pmops_prepare();
>          }
>          dpm_suspend() {
>              amdgpu_pmops_suspend();
>          }
>      }
> }
>
> If the intention was for it to be available, it would be better to move
> the pm_restrict_gfp_mask() call "into" suspend_devices_and_enter()
> between dpm_prepare() and dpm_suspend() calls.

It was not the intention, but then the "prepare" phase is not expected
to do anything that would prevent swap from working, so it might as
well be disabled later.

Then, it would need to be re-enabled before the "complete" phase which
again shouldn't break anything in principle.