Fixups for cancelled hibernate
Subsystems (6)
cover Fixups for cancelled hibernate
Muhammad Usama Anjun's recent series for being able to cancel the hibernate sequence [1] exposes a bug with amdgpu handling for skipping the thaw step.
Because the thaw step is skipped in most cases, cancelling the hibernate means that the device is left in an inconsistent state.
To fix this allow devices to return -EBUSY and adjust the PM core handling of this case.
Mario Limonciello (3):
PM: Mark device as suspended if it failed to resume
PM: Don't pass up device_resume() -EBUSY errors drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
drivers/base/power/main.c | 7 ++++++- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
Signature · 1 line
2.43.0
RFC 1/3 PM: Mark device as suspended if it failed to resume
From: Mario Limonciello <[email protected]>
If a device failed to resume the PM core treats it as though it succeeded. This could cause state machine problems.
--- drivers/base/power/main.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/base/power/main.c +3 -0
@@ -1104,6 +1104,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)device_unlock (dev);dpm_watchdog_clear (&wd); +if (error) + dev->power.is_suspended = true; + Complete:complete_all (&dev->power.completion);
Signature · 1 line
2.43.0
Re: [RFC 1/3] PM: Mark device as suspended if it failed to resume
Quoted 7 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
If a device failed to resume the PM core treats it as though it succeeded. This could cause state machine problems.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>
Quoted 18 lines
--- drivers/base/power/main.c | 3 +++ 1 file changed, 3 insertions(+)drivers/base/power/main.c +3
-0@@ -1104,6 +1104,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)device_unlock (dev);dpm_watchdog_clear (&wd); +if (error) + dev->power.is_suspended = true; + Complete:complete_all (&dev->power.completion);
Signature · 3 lines
--- Thanks, Usama
Rafael J. Wysocki
maintainerRe: [RFC 1/3] PM: Mark device as suspended if it failed to resume
Quoted 6 lines · Muhammad Usama Anjum
Quoted 4 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
If a device failed to resume the PM core treats it as though it succeeded. This could cause state machine problems.
You need to be very specific here as this is a significant change in behavior.
Quoted 29 lines
Quoted 3 lines
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>Tested-by Muhammad Usama Anjum <[email protected]>Quoted 9 lines
--- drivers/base/power/main.c | 3 +++ 1 file changed, 3 insertions(+)drivers/base/power/main.c +0
-0drivers/base/power/main.c static void device_resume(struct device *dev, pm_message_t state, bool async) · line 1112
Quoted 9 lines
drivers/base/power/main.c +3
-0@@ -1104,6 +1104,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)device_unlock (dev);dpm_watchdog_clear (&wd); +if (error) + dev->power.is_suspended = true; + Complete:complete_all (&dev->power.completion);--
---
Thanks,
Usama
RFC 2/3 PM: Don't pass up device_resume() -EBUSY errors
In reply to Mario Limonciello (AMD)From: Mario Limonciello <[email protected]>
If a device resume returns -EBUSY the device resume sequence has been skipped. Don't show errors for this or pass it up to async resume. If resume is run again in another stage the device should try again.
--- drivers/base/power/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/base/power/main.c +3 -1
@@ -1112,7 +1112,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)TRACE_RESUME (error); -if (error) { +if (error == -EBUSY ) +pm_dev_dbg (dev, state, async ?" async" :"" ); +else if (error) {WRITE_ONCE (async_error, error);dpm_save_failed_dev (dev_name (dev));pm_dev_err (dev, state, async ?" async" :"" , error);
Signature · 1 line
2.43.0
Re: [RFC 2/3] PM: Don't pass up device_resume() -EBUSY errors
Quoted 9 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
If a device resume returns
-EBUSYthe device resume sequence has been skipped. Don't show errors for this or pass it up to async resume. If resume is run again in another stage the device should try again.Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>
Quoted 19 lines
--- drivers/base/power/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/base/power/main.c +3
-1@@ -1112,7 +1112,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)TRACE_RESUME (error); -if (error) { +if (error == -EBUSY ) +pm_dev_dbg (dev, state, async ?" async" :"" ); +else if (error) {WRITE_ONCE (async_error, error);dpm_save_failed_dev (dev_name (dev));pm_dev_err (dev, state, async ?" async" :"" , error);
Signature · 3 lines
--- Thanks, Usama
Rafael J. Wysocki
maintainerRe: [RFC 2/3] PM: Don't pass up device_resume() -EBUSY errors
In reply to Mario Limonciello (AMD)Quoted 5 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
If a device resume returns
-EBUSYthe device resume sequence has been skipped.
Is this actually demonstrably true in all of the cases?
And what about -EAGAIN?
Quoted 28 lines
Don't show errors for this or pass it up to async resume. If resume is run again in another stage the device should try again.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>--- drivers/base/power/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/base/power/main.c +3
-2@@ -1112,7 +1112,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)TRACE_RESUME (error); -if (error) { +if (error == -EBUSY ) +pm_dev_dbg (dev, state, async ?" async" :"" ); +else if (error) {WRITE_ONCE (async_error, error);dpm_save_failed_dev (dev_name (dev));pm_dev_err (dev, state, async ?" async" :"" , error); --2.43.0
Re: [RFC 2/3] PM: Don't pass up device_resume() -EBUSY errors
Quoted 12 lines · Rafael J. Wysocki
Quoted 5 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
If a device resume returns
-EBUSYthe device resume sequence has been skipped.Is this actually demonstrably true in all of the cases?
And what about
-EAGAIN?
I haven't audited codepaths of all drivers to guarantee it to be true for all cases. That's the main reason I wanted to make it RFC - to discuss the idea of a dedicated return code to indicate it was skipped.
Another idea I had is that we could make it return a positive number, and PM core could recognize that as a skip.
So would like your thoughts against the ideas currently presented:
* -EAGAIN
* -EBUSY
* Some other return code
* > 0
Whichever is decided the PM core documentation would need to be updated to match as well.
Quoted 28 lines
Quoted 28 lines
Don't show errors for this or pass it up to async resume. If resume is run again in another stage the device should try again.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>--- drivers/base/power/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/base/power/main.c +3
-2@@ -1112,7 +1112,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)TRACE_RESUME (error); -if (error) { +if (error == -EBUSY ) +pm_dev_dbg (dev, state, async ?" async" :"" ); +else if (error) {WRITE_ONCE (async_error, error);dpm_save_failed_dev (dev_name (dev));pm_dev_err (dev, state, async ?" async" :"" , error); --2.43.0
RFC 3/3 drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
In reply to Mario Limonciello (AMD)From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1 -1
@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;return amdgpu_device_resume (drm_dev, true); }
Signature · 1 line
2.43.0
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 8 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>
Quoted 17 lines
--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;return amdgpu_device_resume (drm_dev, true); }
Signature · 3 lines
--- Thanks, Usama
Rafael J. Wysocki
maintainerRe: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
In reply to Mario Limonciello (AMD)Quoted 18 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 5 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;
So that's why you need the special handling of -EBUSY in the previous patch.
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
Quoted 3 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-1return amdgpu_device_resume (drm_dev, true); } --
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)
Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.
Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.
Rafael J. Wysocki
maintainerRe: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 47 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
Quoted 6 lines
We could obviously go back to dropping this case entirely:
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.
The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Some cleanup would need to be done before thawing user space I suppose.
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device thaw().
That series just exposed a very "easy" way to reproduce this issue.
Rafael J. Wysocki
maintainerRe: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 67 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
Quoted 7 lines
I agree; I think that series would need changes for it.
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 71 lines · Rafael J. Wysocki
Quoted 69 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
Quoted 8 lines
Quoted 5 lines
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
You're right; it doesn't abort, it just is saved to the logs. The state is also not maintained.
Quoted 3 lines
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
That's exactly the reason for the first patch in this series. The state of whether it succeeded isn't recorded. So if thaw non-fatally fails and you've saved state to indicate this then any of the other calls that run can try again.
Rafael J. Wysocki
maintainerRe: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 86 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 71 lines · Rafael J. Wysocki
Quoted 69 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
Quoted 8 lines
Quoted 5 lines
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
You're right; it doesn't abort, it just is saved to the logs. The state is also not maintained.
Quoted 8 lines
Quoted 3 lines
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
That's exactly the reason for the first patch in this series. The state of whether it succeeded isn't recorded. So if thaw non-fatally fails and you've saved state to indicate this then any of the other calls that run can try again.
So long as they are called.
But as I said before, I would save the state in the driver thaw callback and then clear it in the driver poweroff callback and look at it in the driver restore callback. If it is there at that point, poweroff has not run and hibernation is rolling back, so you need to do a "thaw".
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 97 lines · Rafael J. Wysocki
Quoted 86 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 71 lines · Rafael J. Wysocki
Quoted 69 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
Quoted 8 lines
Quoted 5 lines
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
You're right; it doesn't abort, it just is saved to the logs. The state is also not maintained.
Quoted 8 lines
Quoted 3 lines
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
That's exactly the reason for the first patch in this series. The state of whether it succeeded isn't recorded. So if thaw non-fatally fails and you've saved state to indicate this then any of the other calls that run can try again.
Quoted 7 lines
So long as they are called.
But as I said before, I would save the state in the driver thaw callback and then clear it in the driver poweroff callback and look at it in the driver restore callback. If it is there at that point, poweroff has not run and hibernation is rolling back, so you need to do a "thaw".
Are you suggesting that the device driver should directly manipulate dev->power.is_suspended?
I'll do some testing but; I suppose that would work as well without needing to make core changes if you don't see a need for other devices to do this.
Rafael J. Wysocki
maintainerRe: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 103 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 99 lines · Rafael J. Wysocki
Quoted 86 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 71 lines · Rafael J. Wysocki
Quoted 69 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
Quoted 8 lines
Quoted 5 lines
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
You're right; it doesn't abort, it just is saved to the logs. The state is also not maintained.
Quoted 8 lines
Quoted 3 lines
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
That's exactly the reason for the first patch in this series. The state of whether it succeeded isn't recorded. So if thaw non-fatally fails and you've saved state to indicate this then any of the other calls that run can try again.
So long as they are called.
Quoted 8 lines
Quoted 5 lines
But as I said before, I would save the state in the driver thaw callback and then clear it in the driver poweroff callback and look at it in the driver restore callback. If it is there at that point, poweroff has not run and hibernation is rolling back, so you need to do a "thaw".
Are you suggesting that the device driver should directly manipulate
dev->power.is_suspended?
No, it needs to have its own state for that. power.is_suspended should not be manipulated by drivers (or anything other than the core for that matter).
Quoted 3 lines
I'll do some testing but; I suppose that would work as well without needing to make core changes if you don't see a need for other devices to do this.
So long as they don't try to skip the "thaw" actions, I don't.
If there are more drivers wanting to do it, I guess it would be good to have a common approach although at this point I'm not sure how much in common there would be.
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 111 lines · Rafael J. Wysocki
Quoted 109 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 104 lines · Rafael J. Wysocki
Quoted 86 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 71 lines · Rafael J. Wysocki
Quoted 69 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
Quoted 8 lines
Quoted 5 lines
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
You're right; it doesn't abort, it just is saved to the logs. The state is also not maintained.
Quoted 8 lines
Quoted 3 lines
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
That's exactly the reason for the first patch in this series. The state of whether it succeeded isn't recorded. So if thaw non-fatally fails and you've saved state to indicate this then any of the other calls that run can try again.
So long as they are called.
But as I said before, I would save the state in the driver thaw callback and then clear it in the driver poweroff callback and look at it in the driver restore callback. If it is there at that point, poweroff has not run and hibernation is rolling back, so you need to do a "thaw".
Quoted 6 lines
Quoted 2 lines
Are you suggesting that the device driver should directly manipulate
dev->power.is_suspended?No, it needs to have its own state for that. power.is_suspended should not be manipulated by drivers (or anything other than the core for that matter).
That's what I originally thought which is why this series looks like it does.
Quoted 10 lines
Quoted 4 lines
I'll do some testing but; I suppose that would work as well without needing to make core changes if you don't see a need for other devices to do this.
So long as they don't try to skip the "thaw" actions, I don't.
If there are more drivers wanting to do it, I guess it would be good to have a common approach although at this point I'm not sure how much in common there would be.
But so if the state is maintained in the driver dev->power.is_suspended will be FALSE at the end of thaw(). That means that restore() is never called for a cancellation/abort.
So I think the only place to do the cleanup would be in the complete() callback. Do you think that's the best place for this based upon that internal driver state variable?
Rafael J. Wysocki
maintainerRe: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 132 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 111 lines · Rafael J. Wysocki
Quoted 109 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 104 lines · Rafael J. Wysocki
Quoted 86 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 71 lines · Rafael J. Wysocki
Quoted 69 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
Quoted 8 lines
Quoted 5 lines
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
You're right; it doesn't abort, it just is saved to the logs. The state is also not maintained.
Quoted 8 lines
Quoted 3 lines
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
That's exactly the reason for the first patch in this series. The state of whether it succeeded isn't recorded. So if thaw non-fatally fails and you've saved state to indicate this then any of the other calls that run can try again.
So long as they are called.
But as I said before, I would save the state in the driver thaw callback and then clear it in the driver poweroff callback and look at it in the driver restore callback. If it is there at that point, poweroff has not run and hibernation is rolling back, so you need to do a "thaw".
Quoted 6 lines
Quoted 2 lines
Are you suggesting that the device driver should directly manipulate
dev->power.is_suspended?No, it needs to have its own state for that. power.is_suspended should not be manipulated by drivers (or anything other than the core for that matter).
That's what I originally thought which is why this series looks like it does.
Quoted 7 lines
Quoted 4 lines
I'll do some testing but; I suppose that would work as well without needing to make core changes if you don't see a need for other devices to do this.
So long as they don't try to skip the "thaw" actions, I don't.
Quoted 7 lines
Quoted 3 lines
If there are more drivers wanting to do it, I guess it would be good to have a common approach although at this point I'm not sure how much in common there would be.
But so if the state is maintained in the driver
dev->power.is_suspendedwill be FALSE at the end ofthaw(). That means thatrestore()is never called for a cancellation/abort.
OK, I see what you mean.
The failing scenario is when "thaw" leaves the devices in "freeze" and then "poweroff" is not called because the final transition is aborted and so "restore" is not called either and the device remains "frozen".
Quoted 3 lines
So I think the only place to do the cleanup would be in the
complete()callback. Do you think that's the best place for this based upon that internal driver state variable?
It would be if nothing else depended on the device in question, but I somehow suspect that it is not the case.
I think that you need to trigger a "restore" for the "frozen" device in the right order with respect to the rest of dpm_list. I guess you could add a special power.frozen flag that will be set by drivers leaving their devices in a "frozen" state in their "thaw" callback. Then, it could be converted to power.is_suspended in the error path of dpm_suspend() for "poweroff" transitions.
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
Quoted 148 lines · Rafael J. Wysocki
Quoted 132 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 111 lines · Rafael J. Wysocki
Quoted 109 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 104 lines · Rafael J. Wysocki
Quoted 86 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 71 lines · Rafael J. Wysocki
Quoted 69 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 51 lines · Rafael J. Wysocki
Quoted 49 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering () && !pm_hibernation_mode_is_suspend ()) -return 0 ; +return -EBUSY ;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
Quoted 7 lines
Quoted 4 lines
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
Quoted 8 lines
Quoted 5 lines
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Device thaw errors don't abort anything AFAICS.
You're right; it doesn't abort, it just is saved to the logs. The state is also not maintained.
Quoted 8 lines
Quoted 3 lines
What can happen though is that another device may abort the final "power off" transition, which is one of the reasons why I think that rolling it back is generally hard.
That's exactly the reason for the first patch in this series. The state of whether it succeeded isn't recorded. So if thaw non-fatally fails and you've saved state to indicate this then any of the other calls that run can try again.
So long as they are called.
But as I said before, I would save the state in the driver thaw callback and then clear it in the driver poweroff callback and look at it in the driver restore callback. If it is there at that point, poweroff has not run and hibernation is rolling back, so you need to do a "thaw".
Quoted 6 lines
Quoted 2 lines
Are you suggesting that the device driver should directly manipulate
dev->power.is_suspended?No, it needs to have its own state for that. power.is_suspended should not be manipulated by drivers (or anything other than the core for that matter).
That's what I originally thought which is why this series looks like it does.
Quoted 7 lines
Quoted 4 lines
I'll do some testing but; I suppose that would work as well without needing to make core changes if you don't see a need for other devices to do this.
So long as they don't try to skip the "thaw" actions, I don't.
Quoted 7 lines
Quoted 3 lines
If there are more drivers wanting to do it, I guess it would be good to have a common approach although at this point I'm not sure how much in common there would be.
But so if the state is maintained in the driver
dev->power.is_suspendedwill be FALSE at the end ofthaw(). That means thatrestore()is never called for a cancellation/abort.OK, I see what you mean.
The failing scenario is when "thaw" leaves the devices in "freeze" and then "poweroff" is not called because the final transition is aborted and so "restore" is not called either and the device remains "frozen".
Quoted 6 lines
Quoted 3 lines
So I think the only place to do the cleanup would be in the
complete()callback. Do you think that's the best place for this based upon that internal driver state variable?It would be if nothing else depended on the device in question, but I somehow suspect that it is not the case.
Muhammad had a try with this and confirmed it worked on an mobile APU (which has no device dependency)
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +6 -0
@@ -2594,6 +2594,12 @@ static int amdgpu_pmops_prepare(struct device *dev)static void amdgpu_pmops_complete (struct device *dev) { +struct drm_device *drm_dev =dev_get_drvdata (dev); +struct amdgpu_device *adev =drm_to_adev (drm_dev); + +if (adev->in_s4 && adev->in_suspend &&pm_hibernate_is_recovering ()) +amdgpu_device_resume (drm_dev, true); +amdgpu_device_complete (dev_get_drvdata (dev)); }
But I think you're right it won't work in the case of a dGPU because the ordering between HDMI audio and GFX needs to be done properly.
Quoted 7 lines
I think that you need to trigger a "restore" for the "frozen" device in the right order with respect to the rest of
dpm_list. I guess you could add a special power.frozen flag that will be set by drivers leaving their devices in a "frozen" state in their "thaw" callback. Then, it could be converted to power.is_suspended in the error path ofdpm_suspend()for "poweroff" transitions.
OK, I'll have a try with changing the series to do this, thanks.
Re: [RFC 3/3] drm/amd: Return -EBUSY for amdgpu_pmops_thaw() on success
In reply to Mario Limonciello (AMD) (kernel.org)Quoted 56 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 53 lines · Rafael J. Wysocki
Quoted 51 lines · Mario Limonciello (AMD) (kernel.org)
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Mario Limonciello (AMD)
From: Mario Limonciello <[email protected]>
The PM core should be notified that thaw was skipped for the device so that if it's tried to be resumed (such as an aborted hibernate) that it gets another chance to resume.
Cc Muhammad Usama Anjum <[email protected]>Signed-off-by Mario Limonciello <[email protected]>---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +0
-0@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c static int amdgpu_pmops_thaw(struct device *dev) · line 2684
Quoted 4 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1
-1/* do not resume device if it's normal hibernation */ if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend()) - return 0; + return -EBUSY;So that's why you need the special handling of
-EBUSYin the previous patch.Yup.
Quoted 5 lines
I think that you need to save some state in this driver and then use it in subsequent callbacks instead of hacking the core to do what you want.
The problem is the core decides "what" to call and more importantly "when" to call it.
IE if the core thinks that something is thawed it will never call resume, and that's why you end up in a bad place with Muhammad's cancellation series and why I proposed this one to discuss.
We could obviously go back to dropping this case entirely:
if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
But then the display turns on at
thaw(), you do an unnecessary resource eviction, it takes a lot longer if you have a ton of VRAM etc.The cancellation series is at odds with this code path AFAICS because what if hibernation is canceled after the entire thaw transition?
Muhammad - did you test that specific timing of cancelling the hibernate?
Yes, I've tested the cancellations before and after the thaw both.
Quoted 8 lines
Quoted 2 lines
Some cleanup would need to be done before thawing user space I suppose.
I agree; I think that series would need changes for it.
But if you put that series aside, I think this one still has some merit on it's own. If another driver aborted the hibernate, I think the same thing could happen if it happened to run before amdgpu's device
thaw().That series just exposed a very "easy" way to reproduce this issue.
Signature · 3 lines
--- Thanks, Usama