Hibernation broken on machines with radeon/KMS and r300

2010/6/14 Rafael J. Wysocki <[email protected]>:

Quoted 22 lines · Alex Deucher

Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).

Quoted 4 lines

Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.

"Just" turning the screen off (like dpms force off) or turning it off while suspending?

Maybe it could be worth enabling drm.debug option and logging using netconsole or similar...

Signature · 5 lines

Rafał
_______________________________________________
linux-pm mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Alex Deucher

2010-06-14 19:57 UTC #7 lore raw

Quoted 24 lines · Rafael J. Wysocki

Quoted 21 lines · Alex Deucher
Quoted 10 lines · Rafael J. Wysocki
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Quoted 9 lines
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?

Quoted 7 lines

Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.

Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html

Alex

Alex Deucher

2010-06-14 19:57 UTC #8 lore raw

Quoted 24 lines · Rafael J. Wysocki

Quoted 21 lines · Alex Deucher
Quoted 10 lines · Rafael J. Wysocki
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Quoted 9 lines
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?

Quoted 7 lines

Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.

Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html

Alex

Rafael J. Wysocki

author

2010-06-15 13:30 UTC #9 lore raw

Quoted 28 lines · Alex Deucher

Quoted 27 lines · Rafael J. Wysocki
Quoted 21 lines · Alex Deucher
Quoted 10 lines · Rafael J. Wysocki
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Quoted 9 lines
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html

No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.

But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).

Rafael

Rafael J. Wysocki

author

2010-06-15 23:07 UTC #10 lore raw

Quoted 37 lines · Rafael J. Wysocki

Quoted 28 lines · Alex Deucher
Quoted 27 lines · Rafael J. Wysocki
Quoted 21 lines · Alex Deucher
Quoted 10 lines · Rafael J. Wysocki
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Quoted 9 lines
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html

Quoted 7 lines

No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).

I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).

Rafael

Rafael J. Wysocki

author

2010-06-15 23:09 UTC #11 lore raw

Quoted 37 lines · Rafael J. Wysocki

Quoted 28 lines · Alex Deucher
Quoted 27 lines · Rafael J. Wysocki
Quoted 21 lines · Alex Deucher
Quoted 10 lines · Rafael J. Wysocki
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Quoted 9 lines
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html

Quoted 7 lines

No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).

Rafael

Ondrej Zary

2010-06-16 06:11 UTC #12 lore raw

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>

Quoted 39 lines · wrote:

Quoted 39 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.

Quoted 8 lines

Quoted 3 lines
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).

It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522

Signature · 1 line

Ondrej Zary

Rafael J. Wysocki

author

2010-06-16 20:16 UTC #13 lore raw

Quoted 50 lines · Ondrej Zary

Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 43 lines · wrote:
Quoted 42 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).

Quoted 7 lines

Quoted 4 lines
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522

Well, my second affected box appears to hang somewhere in the radeon's suspend routine.

Rafael

Alex Deucher

2010-06-16 20:44 UTC #14 lore raw

Quoted 56 lines · Rafael J. Wysocki

Quoted 55 lines · Ondrej Zary
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).

Quoted 5 lines

Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.

Does the attached patch help?

Alex

From 9d2600fd31c1dda6f41e569fd8aff45c9e5ce54f Mon Sep 17 00:00:00 2001
From: Alex Deucher <[email protected]>
Date: Wed, 16 Jun 2010 16:41:38 -0400
Subject: [PATCH] drm/radeon/kms: only attempt to cancel delayed work if pm method is PM_METHOD_DYNPM

delayed work is only scheduled if pm method is PM_METHOD_DYNPM.

Signed-off-by Alex Deucher <[email protected]>

---
 drivers/gpu/drm/radeon/radeon_pm.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

drivers/gpu/drm/radeon/radeon_pm.c +5 -3

index b9ad366..0751fc8 100644
@@ -407,11 +407,12 @@ static ssize_t radeon_set_pm_method(struct device *dev,
 		mutex_unlock(&rdev->pm.mutex);
 	} else if (strncmp("profile", buf, strlen("profile")) == 0) {
 		mutex_lock(&rdev->pm.mutex);
-		rdev->pm.pm_method = PM_METHOD_PROFILE;
+		if (rdev->pm.pm_method == PM_METHOD_DYNPM)
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
 		/* disable dynpm */
 		rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		rdev->pm.pm_method = PM_METHOD_PROFILE;
 		mutex_unlock(&rdev->pm.mutex);
 	} else {
 		DRM_ERROR("invalid power method!\n");

@@ -428,7 +429,8 @@ static DEVICE_ATTR(power_method, S_IRUGO | S_IWUSR, radeon_get_pm_method, radeon
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
 	mutex_lock(&rdev->pm.mutex);
-	cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM)
+		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
 	mutex_unlock(&rdev->pm.mutex);
 }

Signature · 1 line

1.7.0.1

Rafael J. Wysocki

author

2010-06-17 16:21 UTC #15 lore raw

Quoted 57 lines · Alex Deucher

Quoted 56 lines · Rafael J. Wysocki
Quoted 55 lines · Ondrej Zary
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?

It helps, but from what I can see in the code, it still has a few problems.

First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because rdev->pm.pm_method cannot change at this point and cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, in which case the handler can do some wrong things and will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to run in case it's already been queued up and it should also be prevented from rearming itself in that case.

Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work_sync() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so it looks like cancel_delayed_work_sync() should be used to make sure it's not going to run again, but calling that cancel_delayed_work_sync() from under the mutex is not a good idea.

Finally, there's a potential deadlock in radeon_pm_fini(), where cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).

So, I think something like the appended patch is needed.

Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h    |    3 +-
 drivers/gpu/drm/radeon/radeon_pm.c |   41 ++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 8 deletions(-)

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================
@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 		mutex_unlock(&rdev->pm.mutex);
 	} else if (strncmp("profile", buf, strlen("profile")) == 0) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
-		rdev->pm.pm_method = PM_METHOD_PROFILE;
+		if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
+		}
 		/* disable dynpm */
 		rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		rdev->pm.pm_method = PM_METHOD_PROFILE;
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 	} else {
 		DRM_ERROR("invalid power method!\n");
 		goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
+	bool flush_wq = false;
+
 	mutex_lock(&rdev->pm.mutex);
-	cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+			rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+		flush_wq = true;
+	}
 	mutex_unlock(&rdev->pm.mutex);
+	if (flush_wq)
+		flush_workqueue(rdev->wq);
 }
 
 void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
 	rdev->pm.current_sclk = rdev->clock.default_sclk;
 	rdev->pm.current_mclk = rdev->clock.default_mclk;
 	rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM
+	    && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+		rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+	}
 	mutex_unlock(&rdev->pm.mutex);
 	radeon_pm_compute_clocks(rdev);
 }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
 void radeon_pm_fini(struct radeon_device *rdev)
 {
 	if (rdev->pm.num_power_states > 1) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
 		if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
 			rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
 			radeon_pm_set_clocks(rdev);
 		} else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
 			/* cancel work */
-			cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
 			/* reset default clocks */
 			rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 			rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 			radeon_pm_set_clocks(rdev);
 		}
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 
 		device_remove_file(rdev->dev, &dev_attr_power_profile);
 		device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
 			radeon_pm_get_dynpm_state(rdev);
 			radeon_pm_set_clocks(rdev);
 		}
+
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 	}
 	mutex_unlock(&rdev->pm.mutex);
 	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-	queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 }
 
 /*

drivers/gpu/drm/radeon/radeon.h +2 -1

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================
@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
 	DYNPM_STATE_DISABLED,
 	DYNPM_STATE_MINIMUM,
 	DYNPM_STATE_PAUSED,
-	DYNPM_STATE_ACTIVE
+	DYNPM_STATE_ACTIVE,
+	DYNPM_STATE_SUSPENDED,
 };
 enum radeon_dynpm_action {
 	DYNPM_ACTION_NONE,

Alex Deucher

2010-06-17 16:40 UTC #16 lore raw

2010/6/17 Rafael J. Wysocki <[email protected]>:

Quoted 85 lines · Alex Deucher

Quoted 57 lines · Rafael J. Wysocki
Quoted 56 lines · Ondrej Zary
Quoted 55 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Alex Deucher
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Alex Deucher
Quoted 1 line
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?
It helps, but from what I can see in the code, it still has a few problems.
First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because rdev->pm.pm_method cannot change at this point and cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, in which case the handler can do some wrong things and will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to run in case it's already been queued up and it should also be prevented from rearming itself in that case.
Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work_sync() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so it looks like cancel_delayed_work_sync() should be used to make sure it's not going to run again, but calling that cancel_delayed_work_sync() from under the mutex is not a good idea.

Quoted 6 lines

Finally, there's a potential deadlock in radeon_pm_fini(), where cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).
So, I think something like the appended patch is needed.

Looks reasonable. Does it fix the suspend issue?

Alex

Quoted 127 lines

Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>

---
drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_pm.c | 41 ++++++++++++++++++++++++++++++-------

 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                mutex_unlock(&rdev->pm.mutex);
        } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
                /* disable dynpm */
                rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
        } else {
                DRM_ERROR("invalid power method!\n");
                goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
  void radeon_pm_suspend(struct radeon_device *rdev)
  {
+       bool flush_wq = false;
+
        mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
        mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
  }
 
  void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
        rdev->pm.current_sclk = rdev->clock.default_sclk;
        rdev->pm.current_mclk = rdev->clock.default_mclk;
        rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
        mutex_unlock(&rdev->pm.mutex);
        radeon_pm_compute_clocks(rdev);
  }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
  void radeon_pm_fini(struct radeon_device *rdev)
  {
        if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
                if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                        rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                        radeon_pm_set_clocks(rdev);
                } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                        /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                        /* reset default clocks */
                        rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                        rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                        radeon_pm_set_clocks(rdev);
                }
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
                device_remove_file(rdev->dev, &dev_attr_power_profile);
                device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                        radeon_pm_get_dynpm_state(rdev);
                        radeon_pm_set_clocks(rdev);
                }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
        }
        mutex_unlock(&rdev->pm.mutex);
        ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
  }
 
  /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
        DYNPM_STATE_DISABLED,
        DYNPM_STATE_MINIMUM,
        DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
  };
  enum radeon_dynpm_action {
        DYNPM_ACTION_NONE,

Rafael J. Wysocki

author

2010-06-17 19:14 UTC #17 lore raw

Quoted 86 lines · Alex Deucher

2010/6/17 Rafael J. Wysocki <[email protected]>:
Quoted 85 lines · Alex Deucher
Quoted 57 lines · Rafael J. Wysocki
Quoted 56 lines · Ondrej Zary
Quoted 55 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Alex Deucher
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Alex Deucher
Quoted 1 line
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?
It helps, but from what I can see in the code, it still has a few problems.
First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because rdev->pm.pm_method cannot change at this point and cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, in which case the handler can do some wrong things and will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to run in case it's already been queued up and it should also be prevented from rearming itself in that case.
Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work_sync() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so it looks like cancel_delayed_work_sync() should be used to make sure it's not going to run again, but calling that cancel_delayed_work_sync() from under the mutex is not a good idea.

Quoted 8 lines

Quoted 6 lines
Finally, there's a potential deadlock in radeon_pm_fini(), where cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).
So, I think something like the appended patch is needed.
Looks reasonable. Does it fix the suspend issue?

Do you mean the $subject one? Yes, it does.

Rafael

Quoted 126 lines

Quoted 124 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h    |    3 +-
 drivers/gpu/drm/radeon/radeon_pm.c |   41 ++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
               rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
               mutex_unlock(&rdev->pm.mutex);
       } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
               mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
               /* disable dynpm */
               rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
               rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
               mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
       } else {
               DRM_ERROR("invalid power method!\n");
               goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
+       bool flush_wq = false;
+
       mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
       mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
 }
 
 void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
       rdev->pm.current_sclk = rdev->clock.default_sclk;
       rdev->pm.current_mclk = rdev->clock.default_mclk;
       rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
       mutex_unlock(&rdev->pm.mutex);
       radeon_pm_compute_clocks(rdev);
 }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
 void radeon_pm_fini(struct radeon_device *rdev)
 {
       if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
               mutex_lock(&rdev->pm.mutex);
               if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                       rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                       radeon_pm_set_clocks(rdev);
               } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                       /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                       /* reset default clocks */
                       rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                       rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                       radeon_pm_set_clocks(rdev);
               }
               mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
               device_remove_file(rdev->dev, &dev_attr_power_profile);
               device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                       radeon_pm_get_dynpm_state(rdev);
                       radeon_pm_set_clocks(rdev);
               }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
       }
       mutex_unlock(&rdev->pm.mutex);
       ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 }
 
 /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
       DYNPM_STATE_DISABLED,
       DYNPM_STATE_MINIMUM,
       DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
 };
 enum radeon_dynpm_action {
       DYNPM_ACTION_NONE,

Rafael J. Wysocki

author

2010-06-17 19:15 UTC #18 lore raw

Quoted 86 lines · Alex Deucher

2010/6/17 Rafael J. Wysocki <[email protected]>:
Quoted 85 lines · Alex Deucher
Quoted 57 lines · Rafael J. Wysocki
Quoted 56 lines · Ondrej Zary
Quoted 55 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Alex Deucher
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Alex Deucher
Quoted 1 line
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?
It helps, but from what I can see in the code, it still has a few problems.
First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because rdev->pm.pm_method cannot change at this point and cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, in which case the handler can do some wrong things and will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to run in case it's already been queued up and it should also be prevented from rearming itself in that case.
Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work_sync() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so it looks like cancel_delayed_work_sync() should be used to make sure it's not going to run again, but calling that cancel_delayed_work_sync() from under the mutex is not a good idea.

Quoted 8 lines

Quoted 6 lines
Finally, there's a potential deadlock in radeon_pm_fini(), where cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).
So, I think something like the appended patch is needed.
Looks reasonable. Does it fix the suspend issue?

Do you mean the $subject one? Yes, it does.

Rafael

Quoted 126 lines

Quoted 124 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h    |    3 +-
 drivers/gpu/drm/radeon/radeon_pm.c |   41 ++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
               rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
               mutex_unlock(&rdev->pm.mutex);
       } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
               mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
               /* disable dynpm */
               rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
               rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
               mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
       } else {
               DRM_ERROR("invalid power method!\n");
               goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
+       bool flush_wq = false;
+
       mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
       mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
 }
 
 void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
       rdev->pm.current_sclk = rdev->clock.default_sclk;
       rdev->pm.current_mclk = rdev->clock.default_mclk;
       rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
       mutex_unlock(&rdev->pm.mutex);
       radeon_pm_compute_clocks(rdev);
 }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
 void radeon_pm_fini(struct radeon_device *rdev)
 {
       if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
               mutex_lock(&rdev->pm.mutex);
               if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                       rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                       radeon_pm_set_clocks(rdev);
               } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                       /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                       /* reset default clocks */
                       rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                       rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                       radeon_pm_set_clocks(rdev);
               }
               mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
               device_remove_file(rdev->dev, &dev_attr_power_profile);
               device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                       radeon_pm_get_dynpm_state(rdev);
                       radeon_pm_set_clocks(rdev);
               }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
       }
       mutex_unlock(&rdev->pm.mutex);
       ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 }
 
 /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
       DYNPM_STATE_DISABLED,
       DYNPM_STATE_MINIMUM,
       DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
 };
 enum radeon_dynpm_action {
       DYNPM_ACTION_NONE,

Alex Deucher

Reviewed-by

2010-06-17 19:40 UTC #19 lore raw

Quoted 91 lines · Rafael J. Wysocki

Quoted 90 lines · Alex Deucher
2010/6/17 Rafael J. Wysocki <[email protected]>:
Quoted 89 lines · Alex Deucher
Quoted 57 lines · Rafael J. Wysocki
Quoted 56 lines · Ondrej Zary
Quoted 55 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Alex Deucher
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Alex Deucher
Quoted 1 line
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?
It helps, but from what I can see in the code, it still has a few problems.
First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because rdev->pm.pm_method cannot change at this point and cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, in which case the handler can do some wrong things and will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to run in case it's already been queued up and it should also be prevented from rearming itself in that case.
Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work_sync() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so it looks like cancel_delayed_work_sync() should be used to make sure it's not going to run again, but calling that cancel_delayed_work_sync() from under the mutex is not a good idea.
Finally, there's a potential deadlock in radeon_pm_fini(), where cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
So, I think something like the appended patch is needed.
Looks reasonable. Does it fix the suspend issue?
Do you mean the $subject one? Yes, it does.

Great. Thanks for fixing that up.

Quoted 132 lines

Rafael

Quoted 126 lines

Quoted 124 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_pm.c | 41 ++++++++++++++++++++++++++++++-------

 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                mutex_unlock(&rdev->pm.mutex);
        } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
                /* disable dynpm */
                rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
        } else {
                DRM_ERROR("invalid power method!\n");
                goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
  void radeon_pm_suspend(struct radeon_device *rdev)
  {
+       bool flush_wq = false;
+
        mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
        mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
  }
 
  void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
        rdev->pm.current_sclk = rdev->clock.default_sclk;
        rdev->pm.current_mclk = rdev->clock.default_mclk;
        rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
        mutex_unlock(&rdev->pm.mutex);
        radeon_pm_compute_clocks(rdev);
  }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
  void radeon_pm_fini(struct radeon_device *rdev)
  {
        if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
                if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                        rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                        radeon_pm_set_clocks(rdev);
                } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                        /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                        /* reset default clocks */
                        rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                        rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                        radeon_pm_set_clocks(rdev);
                }
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
                device_remove_file(rdev->dev, &dev_attr_power_profile);
                device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                        radeon_pm_get_dynpm_state(rdev);
                        radeon_pm_set_clocks(rdev);
                }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
        }
        mutex_unlock(&rdev->pm.mutex);
        ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
  }
 
  /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
        DYNPM_STATE_DISABLED,
        DYNPM_STATE_MINIMUM,
        DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
  };
  enum radeon_dynpm_action {
        DYNPM_ACTION_NONE,

Rafael J. Wysocki

author

2010-06-17 23:02 UTC #20 lore raw

PATCH 1/1 DRM / radeon / KMS: Fix hibernation regression related to radeon PM (was: Re: [Regression, post-2.6.34] Hibernation broken on machines with radeon/KMS and r300)

From: Rafael J. Wysocki <[email protected]>

There is a regression from 2.6.34 related to the recent radeon power management changes, caused by attempting to cancel a delayed work item that's never been scheduled. However, the code as is has some other issues potentially leading to visible problems.

First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, so the handler can do some wrong things if it wins the race and in that case it will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to return in case it's already been queued up for execution. Also, it should be prevented from rearming itself in that case.

Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so again the work handler should be prevented from rearming itself in that case..

Finally, there's a potential deadlock in radeon_pm_fini(), because cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).

Fix the issues described above.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h    |    3 +-
 drivers/gpu/drm/radeon/radeon_pm.c |   41 ++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 8 deletions(-)

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================
@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 		mutex_unlock(&rdev->pm.mutex);
 	} else if (strncmp("profile", buf, strlen("profile")) == 0) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
-		rdev->pm.pm_method = PM_METHOD_PROFILE;
+		if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
+		}
 		/* disable dynpm */
 		rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		rdev->pm.pm_method = PM_METHOD_PROFILE;
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 	} else {
 		DRM_ERROR("invalid power method!\n");
 		goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
+	bool flush_wq = false;
+
 	mutex_lock(&rdev->pm.mutex);
-	cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+			rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+		flush_wq = true;
+	}
 	mutex_unlock(&rdev->pm.mutex);
+	if (flush_wq)
+		flush_workqueue(rdev->wq);
 }
 
 void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
 	rdev->pm.current_sclk = rdev->clock.default_sclk;
 	rdev->pm.current_mclk = rdev->clock.default_mclk;
 	rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM
+	    && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+		rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+	}
 	mutex_unlock(&rdev->pm.mutex);
 	radeon_pm_compute_clocks(rdev);
 }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
 void radeon_pm_fini(struct radeon_device *rdev)
 {
 	if (rdev->pm.num_power_states > 1) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
 		if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
 			rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
 			radeon_pm_set_clocks(rdev);
 		} else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
 			/* cancel work */
-			cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
 			/* reset default clocks */
 			rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 			rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 			radeon_pm_set_clocks(rdev);
 		}
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 
 		device_remove_file(rdev->dev, &dev_attr_power_profile);
 		device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
 			radeon_pm_get_dynpm_state(rdev);
 			radeon_pm_set_clocks(rdev);
 		}
+
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 	}
 	mutex_unlock(&rdev->pm.mutex);
 	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-	queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 }
 
 /*

drivers/gpu/drm/radeon/radeon.h +2 -1

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================
@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
 	DYNPM_STATE_DISABLED,
 	DYNPM_STATE_MINIMUM,
 	DYNPM_STATE_PAUSED,
-	DYNPM_STATE_ACTIVE
+	DYNPM_STATE_ACTIVE,
+	DYNPM_STATE_SUSPENDED,
 };
 enum radeon_dynpm_action {
 	DYNPM_ACTION_NONE,

Matt Turner

2010-06-17 23:09 UTC #21 lore raw

Re: [PATCH] DRM / radeon / KMS: Fix hibernation regression related to radeon PM (was: Re: [Regression, post-2.6.34] Hibernation broken on machines with radeon/KMS and r300)

Quoted 155 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so again the work handler should be prevented from rearming itself in that case..

Fix the issues described above.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_pm.c | 41 ++++++++++++++++++++++++++++++-------

 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                mutex_unlock(&rdev->pm.mutex);
        } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
                /* disable dynpm */
                rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
        } else {
                DRM_ERROR("invalid power method!\n");
                goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
  void radeon_pm_suspend(struct radeon_device *rdev)
  {
+       bool flush_wq = false;
+
        mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
        mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
  }
 
  void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
        rdev->pm.current_sclk = rdev->clock.default_sclk;
        rdev->pm.current_mclk = rdev->clock.default_mclk;
        rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
        mutex_unlock(&rdev->pm.mutex);
        radeon_pm_compute_clocks(rdev);
  }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
  void radeon_pm_fini(struct radeon_device *rdev)
  {
        if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
                if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                        rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                        radeon_pm_set_clocks(rdev);
                } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                        /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                        /* reset default clocks */
                        rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                        rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                        radeon_pm_set_clocks(rdev);
                }
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
                device_remove_file(rdev->dev, &dev_attr_power_profile);
                device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                        radeon_pm_get_dynpm_state(rdev);
                        radeon_pm_set_clocks(rdev);
                }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
        }
        mutex_unlock(&rdev->pm.mutex);
        ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
  }
 
  /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
        DYNPM_STATE_DISABLED,
        DYNPM_STATE_MINIMUM,
        DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
  };
  enum radeon_dynpm_action {
        DYNPM_ACTION_NONE,

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Probably want to shorten the commit title a bit.

Matt

Rafael J. Wysocki

author

2010-06-18 14:47 UTC #22 lore raw

Re: [PATCH] DRM / radeon / KMS: Fix hibernation regression related to radeon PM (was: Re: [Regression, post-2.6.34] Hibernation broken on machines with radeon/KMS and r300)

Quoted 2 lines · Matt Turner

Quoted 1 line · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>

...

Quoted 3 lines

Quoted 1 line
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Probably want to shorten the commit title a bit.

In fact the title is just "DRM / radeon / KMS: Fix hibernation regression related to radeon PM", the rest is to preserve the context.

Thanks,
Rafael

Rafael J. Wysocki

author

2010-06-18 14:47 UTC #23 lore raw

Re: [PATCH] DRM / radeon / KMS: Fix hibernation regression related to radeon PM (was: Re: [Regression, post-2.6.34] Hibernation broken on machines with radeon/KMS and r300)

In reply to Matt Turner

Quoted 2 lines · Matt Turner

Quoted 1 line · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>

...

Quoted 3 lines

Quoted 1 line
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Probably want to shorten the commit title a bit.

In fact the title is just "DRM / radeon / KMS: Fix hibernation regression related to radeon PM", the rest is to preserve the context.

Thanks,
Rafael

Matt Turner

2010-06-17 23:09 UTC #24 lore raw

Re: [PATCH] DRM / radeon / KMS: Fix hibernation regression related to radeon PM (was: Re: [Regression, post-2.6.34] Hibernation broken on machines with radeon/KMS and r300)

Quoted 155 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so again the work handler should be prevented from rearming itself in that case..

Fix the issues described above.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_pm.c | 41 ++++++++++++++++++++++++++++++-------

 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                mutex_unlock(&rdev->pm.mutex);
        } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
                /* disable dynpm */
                rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
        } else {
                DRM_ERROR("invalid power method!\n");
                goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
  void radeon_pm_suspend(struct radeon_device *rdev)
  {
+       bool flush_wq = false;
+
        mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
        mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
  }
 
  void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
        rdev->pm.current_sclk = rdev->clock.default_sclk;
        rdev->pm.current_mclk = rdev->clock.default_mclk;
        rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
        mutex_unlock(&rdev->pm.mutex);
        radeon_pm_compute_clocks(rdev);
  }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
  void radeon_pm_fini(struct radeon_device *rdev)
  {
        if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
                if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                        rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                        radeon_pm_set_clocks(rdev);
                } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                        /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                        /* reset default clocks */
                        rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                        rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                        radeon_pm_set_clocks(rdev);
                }
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
                device_remove_file(rdev->dev, &dev_attr_power_profile);
                device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                        radeon_pm_get_dynpm_state(rdev);
                        radeon_pm_set_clocks(rdev);
                }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
        }
        mutex_unlock(&rdev->pm.mutex);
        ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
  }
 
  /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
        DYNPM_STATE_DISABLED,
        DYNPM_STATE_MINIMUM,
        DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
  };
  enum radeon_dynpm_action {
        DYNPM_ACTION_NONE,

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Probably want to shorten the commit title a bit.

Matt

Rafael J. Wysocki

author

2010-06-17 23:02 UTC #25 lore raw

PATCH 1/1 DRM / radeon / KMS: Fix hibernation regression related to radeon PM (was: Re: [Regression, post-2.6.34] Hibernation broken on machines with radeon/KMS and r300)

From: Rafael J. Wysocki <[email protected]>

Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so again the work handler should be prevented from rearming itself in that case..

Fix the issues described above.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h    |    3 +-
 drivers/gpu/drm/radeon/radeon_pm.c |   41 ++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 8 deletions(-)

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================
@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 		mutex_unlock(&rdev->pm.mutex);
 	} else if (strncmp("profile", buf, strlen("profile")) == 0) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
-		rdev->pm.pm_method = PM_METHOD_PROFILE;
+		if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
+		}
 		/* disable dynpm */
 		rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		rdev->pm.pm_method = PM_METHOD_PROFILE;
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 	} else {
 		DRM_ERROR("invalid power method!\n");
 		goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
+	bool flush_wq = false;
+
 	mutex_lock(&rdev->pm.mutex);
-	cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+			rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+		flush_wq = true;
+	}
 	mutex_unlock(&rdev->pm.mutex);
+	if (flush_wq)
+		flush_workqueue(rdev->wq);
 }
 
 void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
 	rdev->pm.current_sclk = rdev->clock.default_sclk;
 	rdev->pm.current_mclk = rdev->clock.default_mclk;
 	rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM
+	    && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+		rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+	}
 	mutex_unlock(&rdev->pm.mutex);
 	radeon_pm_compute_clocks(rdev);
 }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
 void radeon_pm_fini(struct radeon_device *rdev)
 {
 	if (rdev->pm.num_power_states > 1) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
 		if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
 			rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
 			radeon_pm_set_clocks(rdev);
 		} else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
 			/* cancel work */
-			cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
 			/* reset default clocks */
 			rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 			rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 			radeon_pm_set_clocks(rdev);
 		}
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 
 		device_remove_file(rdev->dev, &dev_attr_power_profile);
 		device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
 			radeon_pm_get_dynpm_state(rdev);
 			radeon_pm_set_clocks(rdev);
 		}
+
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 	}
 	mutex_unlock(&rdev->pm.mutex);
 	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-	queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 }
 
 /*

drivers/gpu/drm/radeon/radeon.h +2 -1

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================
@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
 	DYNPM_STATE_DISABLED,
 	DYNPM_STATE_MINIMUM,
 	DYNPM_STATE_PAUSED,
-	DYNPM_STATE_ACTIVE
+	DYNPM_STATE_ACTIVE,
+	DYNPM_STATE_SUSPENDED,
 };
 enum radeon_dynpm_action {
 	DYNPM_ACTION_NONE,

Alex Deucher

Reviewed-by

2010-06-17 19:40 UTC #26 lore raw

Quoted 91 lines · Rafael J. Wysocki

Quoted 90 lines · Alex Deucher
2010/6/17 Rafael J. Wysocki <[email protected]>:
Quoted 89 lines · Alex Deucher
Quoted 57 lines · Rafael J. Wysocki
Quoted 56 lines · Ondrej Zary
Quoted 55 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Alex Deucher
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Alex Deucher
Quoted 1 line
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?
It helps, but from what I can see in the code, it still has a few problems.
First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because rdev->pm.pm_method cannot change at this point and cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, in which case the handler can do some wrong things and will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to run in case it's already been queued up and it should also be prevented from rearming itself in that case.
Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work_sync() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so it looks like cancel_delayed_work_sync() should be used to make sure it's not going to run again, but calling that cancel_delayed_work_sync() from under the mutex is not a good idea.
Finally, there's a potential deadlock in radeon_pm_fini(), where cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
So, I think something like the appended patch is needed.
Looks reasonable. Does it fix the suspend issue?
Do you mean the $subject one? Yes, it does.

Great. Thanks for fixing that up.

Quoted 132 lines

Rafael

Quoted 126 lines

Quoted 124 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_pm.c | 41 ++++++++++++++++++++++++++++++-------

 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                mutex_unlock(&rdev->pm.mutex);
        } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
                /* disable dynpm */
                rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
        } else {
                DRM_ERROR("invalid power method!\n");
                goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
  void radeon_pm_suspend(struct radeon_device *rdev)
  {
+       bool flush_wq = false;
+
        mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
        mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
  }
 
  void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
        rdev->pm.current_sclk = rdev->clock.default_sclk;
        rdev->pm.current_mclk = rdev->clock.default_mclk;
        rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
        mutex_unlock(&rdev->pm.mutex);
        radeon_pm_compute_clocks(rdev);
  }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
  void radeon_pm_fini(struct radeon_device *rdev)
  {
        if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
                if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                        rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                        radeon_pm_set_clocks(rdev);
                } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                        /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                        /* reset default clocks */
                        rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                        rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                        radeon_pm_set_clocks(rdev);
                }
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
                device_remove_file(rdev->dev, &dev_attr_power_profile);
                device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                        radeon_pm_get_dynpm_state(rdev);
                        radeon_pm_set_clocks(rdev);
                }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
        }
        mutex_unlock(&rdev->pm.mutex);
        ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
  }
 
  /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
        DYNPM_STATE_DISABLED,
        DYNPM_STATE_MINIMUM,
        DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
  };
  enum radeon_dynpm_action {
        DYNPM_ACTION_NONE,

Alex Deucher

2010-06-17 16:40 UTC #27 lore raw

2010/6/17 Rafael J. Wysocki <[email protected]>:

Quoted 85 lines · Alex Deucher

Quoted 57 lines · Rafael J. Wysocki
Quoted 56 lines · Ondrej Zary
Quoted 55 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Alex Deucher
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Alex Deucher
Quoted 1 line
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?
It helps, but from what I can see in the code, it still has a few problems.
First, the mutex around cancel_delayed_work() in radeon_pm_suspend() doesn't really serve any purpose, because rdev->pm.pm_method cannot change at this point and cancel_delayed_work() only tries to delete the work's timer. Moreover, it doesn't prevent the work handler from running, in which case the handler can do some wrong things and will rearm itself to do some more wrong things going forward. So, I think it's better to wait for the handler to run in case it's already been queued up and it should also be prevented from rearming itself in that case.
Second, in radeon_set_pm_method() the cancel_delayed_work() is not sufficient to prevent the work handler from running and queing up itself for the next run (the failure scenario is that cancel_delayed_work_sync() returns 0, so the handler is run, it waits on the mutex and then rearms itself after the mutex has been released), so it looks like cancel_delayed_work_sync() should be used to make sure it's not going to run again, but calling that cancel_delayed_work_sync() from under the mutex is not a good idea.

Quoted 6 lines

Finally, there's a potential deadlock in radeon_pm_fini(), where cancel_delayed_work_sync() is called under rdev->pm.mutex, but the work handler tries to acquire the same mutex (if it wins the race).
So, I think something like the appended patch is needed.

Looks reasonable. Does it fix the suspend issue?

Alex

Quoted 127 lines

Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>

---
drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_pm.c | 41 ++++++++++++++++++++++++++++++-------

 2 files changed, 36 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                mutex_unlock(&rdev->pm.mutex);
        } else if (strncmp("profile", buf, strlen("profile")) == 0) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
-               rdev->pm.pm_method = PM_METHOD_PROFILE;
+               if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
+               }
                /* disable dynpm */
                rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               rdev->pm.pm_method = PM_METHOD_PROFILE;
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
        } else {
                DRM_ERROR("invalid power method!\n");
                goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
  void radeon_pm_suspend(struct radeon_device *rdev)
  {
+       bool flush_wq = false;
+
        mutex_lock(&rdev->pm.mutex);
-       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+               cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+               if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+                       rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+               flush_wq = true;
+       }
        mutex_unlock(&rdev->pm.mutex);
+       if (flush_wq)
+               flush_workqueue(rdev->wq);
  }
 
  void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
        rdev->pm.current_sclk = rdev->clock.default_sclk;
        rdev->pm.current_mclk = rdev->clock.default_mclk;
        rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+       if (rdev->pm.pm_method == PM_METHOD_DYNPM
+           && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+               rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+       }
        mutex_unlock(&rdev->pm.mutex);
        radeon_pm_compute_clocks(rdev);
  }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
  void radeon_pm_fini(struct radeon_device *rdev)
  {
        if (rdev->pm.num_power_states > 1) {
+               bool flush_wq = false;
+
                mutex_lock(&rdev->pm.mutex);
                if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
                        rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
                        radeon_pm_set_clocks(rdev);
                } else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
                        /* cancel work */
-                       cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+                       cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+                       flush_wq = true;
                        /* reset default clocks */
                        rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
                        rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
                        radeon_pm_set_clocks(rdev);
                }
                mutex_unlock(&rdev->pm.mutex);
+               if (flush_wq)
+                       flush_workqueue(rdev->wq);
 
                device_remove_file(rdev->dev, &dev_attr_power_profile);
                device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
                        radeon_pm_get_dynpm_state(rdev);
                        radeon_pm_set_clocks(rdev);
                }
+
+               queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
        }
        mutex_unlock(&rdev->pm.mutex);
        ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-       queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-                                       msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
  }
 
  /*

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================

drivers/gpu/drm/radeon/radeon.h +2 -1

@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
        DYNPM_STATE_DISABLED,
        DYNPM_STATE_MINIMUM,
        DYNPM_STATE_PAUSED,
-       DYNPM_STATE_ACTIVE
+       DYNPM_STATE_ACTIVE,
+       DYNPM_STATE_SUSPENDED,
  };
  enum radeon_dynpm_action {
        DYNPM_ACTION_NONE,

Rafael J. Wysocki

author

2010-06-17 16:21 UTC #28 lore raw

Quoted 57 lines · Alex Deucher

Quoted 56 lines · Rafael J. Wysocki
Quoted 55 lines · Ondrej Zary
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.
Does the attached patch help?

It helps, but from what I can see in the code, it still has a few problems.

So, I think something like the appended patch is needed.

Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h    |    3 +-
 drivers/gpu/drm/radeon/radeon_pm.c |   41 ++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 8 deletions(-)

drivers/gpu/drm/radeon/radeon_pm.c +34 -7

Index: linux-2.6/drivers/gpu/drm/radeon/radeon_pm.c
===================================================================
@@ -397,13 +397,20 @@ static ssize_t radeon_set_pm_method(stru
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 		mutex_unlock(&rdev->pm.mutex);
 	} else if (strncmp("profile", buf, strlen("profile")) == 0) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
-		rdev->pm.pm_method = PM_METHOD_PROFILE;
+		if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
+		}
 		/* disable dynpm */
 		rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		rdev->pm.pm_method = PM_METHOD_PROFILE;
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 	} else {
 		DRM_ERROR("invalid power method!\n");
 		goto fail;

@@ -418,9 +425,18 @@ static DEVICE_ATTR(power_method, S_IRUGO
 
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
+	bool flush_wq = false;
+
 	mutex_lock(&rdev->pm.mutex);
-	cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
+		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		if (rdev->pm.dynpm_state == DYNPM_STATE_ACTIVE)
+			rdev->pm.dynpm_state = DYNPM_STATE_SUSPENDED;
+		flush_wq = true;
+	}
 	mutex_unlock(&rdev->pm.mutex);
+	if (flush_wq)
+		flush_workqueue(rdev->wq);
 }
 
 void radeon_pm_resume(struct radeon_device *rdev)

@@ -432,6 +448,12 @@ void radeon_pm_resume(struct radeon_devi
 	rdev->pm.current_sclk = rdev->clock.default_sclk;
 	rdev->pm.current_mclk = rdev->clock.default_mclk;
 	rdev->pm.current_vddc = rdev->pm.power_state[rdev->pm.default_power_state_index].clock_info[0].voltage.voltage;
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM
+	    && rdev->pm.dynpm_state == DYNPM_STATE_SUSPENDED) {
+		rdev->pm.dynpm_state = DYNPM_STATE_ACTIVE;
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
+	}
 	mutex_unlock(&rdev->pm.mutex);
 	radeon_pm_compute_clocks(rdev);
 }

@@ -486,6 +508,8 @@ int radeon_pm_init(struct radeon_device
 void radeon_pm_fini(struct radeon_device *rdev)
 {
 	if (rdev->pm.num_power_states > 1) {
+		bool flush_wq = false;
+
 		mutex_lock(&rdev->pm.mutex);
 		if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
 			rdev->pm.profile = PM_PROFILE_DEFAULT;

@@ -493,13 +517,16 @@ void radeon_pm_fini(struct radeon_device
 			radeon_pm_set_clocks(rdev);
 		} else if (rdev->pm.pm_method == PM_METHOD_DYNPM) {
 			/* cancel work */
-			cancel_delayed_work_sync(&rdev->pm.dynpm_idle_work);
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+			flush_wq = true;
 			/* reset default clocks */
 			rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 			rdev->pm.dynpm_planned_action = DYNPM_ACTION_DEFAULT;
 			radeon_pm_set_clocks(rdev);
 		}
 		mutex_unlock(&rdev->pm.mutex);
+		if (flush_wq)
+			flush_workqueue(rdev->wq);
 
 		device_remove_file(rdev->dev, &dev_attr_power_profile);
 		device_remove_file(rdev->dev, &dev_attr_power_method);

@@ -720,12 +747,12 @@ static void radeon_dynpm_idle_work_handl
 			radeon_pm_get_dynpm_state(rdev);
 			radeon_pm_set_clocks(rdev);
 		}
+
+		queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
+					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 	}
 	mutex_unlock(&rdev->pm.mutex);
 	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
-
-	queue_delayed_work(rdev->wq, &rdev->pm.dynpm_idle_work,
-					msecs_to_jiffies(RADEON_IDLE_LOOP_MS));
 }
 
 /*

drivers/gpu/drm/radeon/radeon.h +2 -1

Index: linux-2.6/drivers/gpu/drm/radeon/radeon.h
===================================================================
@@ -619,7 +619,8 @@ enum radeon_dynpm_state {
 	DYNPM_STATE_DISABLED,
 	DYNPM_STATE_MINIMUM,
 	DYNPM_STATE_PAUSED,
-	DYNPM_STATE_ACTIVE
+	DYNPM_STATE_ACTIVE,
+	DYNPM_STATE_SUSPENDED,
 };
 enum radeon_dynpm_action {
 	DYNPM_ACTION_NONE,

Alex Deucher

2010-06-16 20:44 UTC #29 lore raw

Quoted 56 lines · Rafael J. Wysocki

Quoted 55 lines · Ondrej Zary
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 47 lines · wrote:
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
Quoted 7 lines
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).

Quoted 5 lines

Quoted 2 lines
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522
Well, my second affected box appears to hang somewhere in the radeon's suspend routine.

Does the attached patch help?

Alex

delayed work is only scheduled if pm method is PM_METHOD_DYNPM.

Signed-off-by Alex Deucher <[email protected]>

---
 drivers/gpu/drm/radeon/radeon_pm.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

drivers/gpu/drm/radeon/radeon_pm.c +5 -3

index b9ad366..0751fc8 100644
@@ -407,11 +407,12 @@ static ssize_t radeon_set_pm_method(struct device *dev,
 		mutex_unlock(&rdev->pm.mutex);
 	} else if (strncmp("profile", buf, strlen("profile")) == 0) {
 		mutex_lock(&rdev->pm.mutex);
-		rdev->pm.pm_method = PM_METHOD_PROFILE;
+		if (rdev->pm.pm_method == PM_METHOD_DYNPM)
+			cancel_delayed_work(&rdev->pm.dynpm_idle_work);
 		/* disable dynpm */
 		rdev->pm.dynpm_state = DYNPM_STATE_DISABLED;
 		rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
-		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+		rdev->pm.pm_method = PM_METHOD_PROFILE;
 		mutex_unlock(&rdev->pm.mutex);
 	} else {
 		DRM_ERROR("invalid power method!\n");

@@ -428,7 +429,8 @@ static DEVICE_ATTR(power_method, S_IRUGO | S_IWUSR, radeon_get_pm_method, radeon
 void radeon_pm_suspend(struct radeon_device *rdev)
 {
 	mutex_lock(&rdev->pm.mutex);
-	cancel_delayed_work(&rdev->pm.dynpm_idle_work);
+	if (rdev->pm.pm_method == PM_METHOD_DYNPM)
+		cancel_delayed_work(&rdev->pm.dynpm_idle_work);
 	mutex_unlock(&rdev->pm.mutex);
 }

Signature · 1 line

1.7.0.1

Rafael J. Wysocki

author

2010-06-16 20:16 UTC #30 lore raw

In reply to Ondrej Zary

Quoted 50 lines · Ondrej Zary

Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>
Quoted 43 lines · wrote:
Quoted 42 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).

Quoted 7 lines

Quoted 4 lines
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).
It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522

Well, my second affected box appears to hang somewhere in the radeon's suspend routine.

Rafael

Ondrej Zary

2010-06-16 06:11 UTC #31 lore raw

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines · Rafael J. Wysocki
Quoted 3 lines · Alex Deucher
Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Alex Deucher
On Mon, Jun 14, 2010 at 10:53 AM, Rafael J. Wysocki <[email protected]>

Quoted 39 lines · wrote:

Quoted 39 lines
Quoted 27 lines
Quoted 27 lines
Quoted 22 lines
Quoted 14 lines
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Suspend to RAM appears to work fine at least on one of the affected boxes.
Quoted 7 lines
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html
No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.

Quoted 8 lines

Quoted 3 lines
But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).
I've just verified that in fact hibernation works on HP nx6325 with 2.6.35-rc3, but it takes about 55 sec. to suspend the graphics adapter in the "freeze" phase. Surprisingly enough, during suspend to RAM it works normally (as well as in the "poweroff" phase of hibernation).

It takes 2 minutes on RV530:
https://bugzilla.redhat.com/show_bug.cgi?id=586522

Signature · 1 line

Ondrej Zary

Rafael J. Wysocki

author

2010-06-15 13:30 UTC #32 lore raw

Quoted 28 lines · Alex Deucher

Quoted 27 lines · Rafael J. Wysocki
Quoted 21 lines · Alex Deucher
Quoted 10 lines · Rafael J. Wysocki
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Quoted 9 lines
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).
Quoted 1 line
Does it work any better after Dave's last drm pull request?
Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.
Well, I guess the first pm patch changed more than just pm, then.
Does this patch help?
http://lists.freedesktop.org/archives/dri-devel/2010-June/001314.html

No, it doesn't. I try to hibernate, everything works to the point where the screen goes off and the box hangs (solid). Normally, it would turn the screen back on and continue with saving the image.

But, since that happens with the patch above applied, I think it doesn't really pass the suspend phase (IOW, it probably hangs somewhere in the radeon's suspend routine).

Rafael

Rafael J. Wysocki

author

2010-06-14 19:03 UTC #33 lore raw

Quoted 21 lines · Alex Deucher

Quoted 10 lines · Rafael J. Wysocki
Alex, Dave,
I'm afraid hibernation is broken on all machines using radeon/KMS with r300 after commit ce8f53709bf440100cb9d31b1303291551cf517f (drm/radeon/kms/pm: rework power management). At least, I'm able to reproduce the symptom, which is that the machine hangs hard around the point where an image is created (probably during the device thaw phase), on two different boxes with r300 (the output of lspci from one of them is attached for reference, the other one is HP nx6325).
Quoted 9 lines
Suspend to RAM appears to work fine at least on one of the affected boxes.
Unfortunately, the commit above changes a lot of code and it's not too easy to figure out what's wrong with it and I didn't have the time to look more into details of this failure. However, it looks like you use .suspend() and .resume() callbacks as .freeze() and .thaw() which may not be 100% correct (in fact it looks like the "legacy" PCI suspend/resume is used, which is not recommended any more).

Quoted 1 line

Does it work any better after Dave's last drm pull request?

Nope. The symptom is slightly different, though, because now it hangs after turning off the screen.

Quoted 2 lines

With the latest changes, pm should not be a factor unless it's explicitly enabled via sysfs.

Well, I guess the first pm patch changed more than just pm, then.

Rafael

Alex Deucher

2010-06-14 16:00 UTC #34 lore raw