FAILED: patch "[PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled" failed to apply to 5.14-stable tree

PATCH [email protected] 3 participants 8 messages Sep 16, 2021

Subsystems (3)

Stable branch DRM drivers Radeon and amdgpu DRM drivers

lore

Share & tools

lore

[email protected]

author maintainer

2021-09-16 13:05 UTC #0 lore raw

The patch below does not apply to the 5.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 90a9266269eb9f71af1f323c33e1dca53527bd22 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michel=20D=C3=A4nzer?= <[email protected]>
Date: Tue, 17 Aug 2021 10:23:25 +0200
Subject: [PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

schedule_delayed_work does not push back the work if it was already scheduled before, so amdgpu_device_delay_enable_gfx_off ran ~100 ms after the first time GFXOFF was disabled and re-enabled, even if GFXOFF was disabled and re-enabled again during those 100 ms.

This resulted in frame drops / stutter with the upcoming mutter 41 release on Navi 14, due to constantly enabling GFXOFF in the HW and disabling it again (for getting the GPU clock counter).

To fix this, call cancel_delayed_work_sync when the disable count transitions from 0 to 1, and only schedule the delayed work on the reverse transition, not if the disable count was already 0. This makes sure the delayed work doesn't run at unexpected times, and allows it to be lock-free.

v2:
* Use cancel_delayed_work_sync & mutex_trylock instead of mod_delayed_work.
v3:
* Make amdgpu_device_delay_enable_gfx_off lock-free (Christian König) v4:
* Fix race condition between amdgpu_gfx_off_ctrl incrementing adev->gfx.gfx_off_req_count and amdgpu_device_delay_enable_gfx_off checking for it to be 0 (Evan Quan)

Cc [email protected]

Reviewed-by Evan Quan <[email protected]>

Reviewed-by Lijo Lazar <[email protected]> # v3

Acked-by Christian König <[email protected]> # v3

Signed-off-by Michel Dänzer <[email protected]>

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

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +5 -6

index 41cc00e489ac..41c6b3aacd37 100644
@@ -2829,12 +2829,11 @@ static void amdgpu_device_delay_enable_gfx_off(struct work_struct *work)
 	struct amdgpu_device *adev =
 		container_of(work, struct amdgpu_device, gfx.gfx_off_delay_work.work);
 
-	mutex_lock(&adev->gfx.gfx_off_mutex);
-	if (!adev->gfx.gfx_off_state && !adev->gfx.gfx_off_req_count) {
-		if (!amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, true))
-			adev->gfx.gfx_off_state = true;
-	}
-	mutex_unlock(&adev->gfx.gfx_off_mutex);
+	WARN_ON_ONCE(adev->gfx.gfx_off_state);
+	WARN_ON_ONCE(adev->gfx.gfx_off_req_count);
+
+	if (!amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, true))
+		adev->gfx.gfx_off_state = true;
 }
 
 /**

drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c +25 ~~-11~~

index e7e9655c5623..e7f06bd0f0cd 100644
@@ -563,24 +563,38 @@ void amdgpu_gfx_off_ctrl(struct amdgpu_device *adev, bool enable)
 
 	mutex_lock(&adev->gfx.gfx_off_mutex);
 
-	if (!enable)
-		adev->gfx.gfx_off_req_count++;
-	else if (adev->gfx.gfx_off_req_count > 0)
+	if (enable) {
+		/* If the count is already 0, it means there's an imbalance bug somewhere.
+		 * Note that the bug may be in a different caller than the one which triggers the
+		 * WARN_ON_ONCE.
+		 */
+		if (WARN_ON_ONCE(adev->gfx.gfx_off_req_count == 0))
+			goto unlock;
+
 		adev->gfx.gfx_off_req_count--;
 
-	if (enable && !adev->gfx.gfx_off_state && !adev->gfx.gfx_off_req_count) {
-		schedule_delayed_work(&adev->gfx.gfx_off_delay_work, GFX_OFF_DELAY_ENABLE);
-	} else if (!enable && adev->gfx.gfx_off_state) {
-		if (!amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, false)) {
-			adev->gfx.gfx_off_state = false;
+		if (adev->gfx.gfx_off_req_count == 0 && !adev->gfx.gfx_off_state)
+			schedule_delayed_work(&adev->gfx.gfx_off_delay_work, GFX_OFF_DELAY_ENABLE);
+	} else {
+		if (adev->gfx.gfx_off_req_count == 0) {
+			cancel_delayed_work_sync(&adev->gfx.gfx_off_delay_work);
+
+			if (adev->gfx.gfx_off_state &&
+			    !amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, false)) {
+				adev->gfx.gfx_off_state = false;
 
-			if (adev->gfx.funcs->init_spm_golden) {
-				dev_dbg(adev->dev, "GFXOFF is disabled, re-init SPM golden settings\n");
-				amdgpu_gfx_init_spm_golden(adev);
+				if (adev->gfx.funcs->init_spm_golden) {
+					dev_dbg(adev->dev,
+						"GFXOFF is disabled, re-init SPM golden settings\n");
+					amdgpu_gfx_init_spm_golden(adev);
+				}
 			}
 		}
+
+		adev->gfx.gfx_off_req_count++;
 	}
 
+unlock:
 	mutex_unlock(&adev->gfx.gfx_off_mutex);
 }

Michel Dänzer

2021-09-16 13:39 UTC #1 lore raw

Quoted 5 lines · On 2021-09-16 15:05, [email protected] wrote:

The patch below does not apply to the 5.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

It's already in 5.14, commit 32bc8f8373d2d6a681c96e4b25dca60d4d1c6016.

Please do backport it to older stable trees.

Signature · 2 lines

Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer

Greg KH

author maintainer

2021-09-16 13:50 UTC #2 lore raw

Quoted 8 lines · Michel Dänzer

Quoted 5 lines · On 2021-09-16 15:05, [email protected] wrote:
The patch below does not apply to the 5.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
It's already in 5.14, commit 32bc8f8373d2d6a681c96e4b25dca60d4d1c6016.

Odd, how were we supposed to know that?

Quoted 1 line

Please do backport it to older stable trees.

The above commit is already in 5.10.62 and 5.13.14.

thanks,

greg k-h

Michel Dänzer

2021-09-16 13:54 UTC #3 lore raw

Quoted 11 lines · Greg KH

Quoted 8 lines · Michel Dänzer
Quoted 5 lines · On 2021-09-16 15:05, [email protected] wrote:
The patch below does not apply to the 5.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
It's already in 5.14, commit 32bc8f8373d2d6a681c96e4b25dca60d4d1c6016.
Odd, how were we supposed to know that?

Looks like the fix was merged separately for 5.14 and 5.15. I don't know how/why that happened. Alex / Christian?

Signature · 2 lines

Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer

Deucher, Alexander

maintainer

2021-09-16 14:42 UTC #4 lore raw

[Public]

Quoted 19 lines

-----Original Message-----
From: Michel Dänzer <[email protected]>
Sent: Thursday, September 16, 2021 9:55 AM
To: Greg KH <[email protected]>
Cc: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Quan, Evan <[email protected]>; Lazar, Lijo <[email protected]>; [email protected]
Subject: Re: FAILED: patch "[PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled" failed to apply to 5.14-stable tree
Quoted 8 lines · Greg KH
Quoted 7 lines · Michel Dänzer
Quoted 5 lines · On 2021-09-16 15:05, [email protected] wrote:
The patch below does not apply to the 5.14-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
It's already in 5.14, commit 32bc8f8373d2d6a681c96e4b25dca60d4d1c6016.
Odd, how were we supposed to know that?
Looks like the fix was merged separately for 5.14 and 5.15. I don't know how/why that happened. Alex / Christian?

The fix already landed in drm-next, but since this was before the merge window, we wanted to make sure the fix also landed in stable, so I cherry-picked it to 5.14. I'm not sure of a better way to handle these sort of cases.

Alex

Quoted 12 lines

--
Earthling Michel Dänzer |
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fredh
at.com%2F&data=04%7C01%7Calexander.deucher%40amd.com%7Ceaf ba6f5f87c49c5fc0b08d9791987a2%7C3dd8961fe4884e608e11a82d994e183d% 7C0%7C0%7C637673972820037953%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100 0&sdata=VQ5VPN9N3%2B0oRSWQDSgwU9D7V6ONdwaE34huiwRYoW8 %3D&reserved=0
Libre software enthusiast | Mesa and X developer

Greg KH

author maintainer

2021-09-16 15:35 UTC #5 lore raw

Quoted 25 lines · Alexander

[Public]
Quoted 23 lines
-----Original Message-----
From: Michel Dänzer <[email protected]>
Sent: Thursday, September 16, 2021 9:55 AM
To: Greg KH <[email protected]>
Cc: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Quan, Evan <[email protected]>; Lazar, Lijo <[email protected]>; [email protected]
Subject: Re: FAILED: patch "[PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled" failed to apply to 5.14-stable tree
Quoted 11 lines · Greg KH
Quoted 8 lines · Michel Dänzer
Quoted 5 lines · On 2021-09-16 15:05, [email protected] wrote:
The patch below does not apply to the 5.14-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
It's already in 5.14, commit 32bc8f8373d2d6a681c96e4b25dca60d4d1c6016.
Odd, how were we supposed to know that?

Quoted 7 lines

Quoted 2 lines
Looks like the fix was merged separately for 5.14 and 5.15. I don't know how/why that happened. Alex / Christian?
The fix already landed in drm-next, but since this was before the merge window, we wanted to make sure the fix also landed in stable, so I cherry-picked it to 5.14. I'm not sure of a better way to handle these sort of cases.

You gotta give me a chance to know what happened. As the drm developers do this a lot, they have been putting the "cherry picked from" line in the patch because they can not merge between branches and keep the git id the same.

So try using the '-x' option to 'git cherry-pick' next time?

thanks,

greg k-h

Deucher, Alexander

maintainer

2021-09-16 16:05 UTC #6 lore raw

[Public]

Quoted 46 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Thursday, September 16, 2021 11:36 AM
To: Deucher, Alexander <[email protected]>
Cc: Michel Dänzer <[email protected]>; Koenig, Christian <[email protected]>; Quan, Evan <[email protected]>; Lazar, Lijo <[email protected]>; [email protected]
Subject: Re: FAILED: patch "[PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled" failed to apply to 5.14-stable tree
Quoted 23 lines · Alexander
[Public]
Quoted 21 lines
-----Original Message-----
From: Michel Dänzer <[email protected]>
Sent: Thursday, September 16, 2021 9:55 AM
To: Greg KH <[email protected]>
Cc: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Quan, Evan <[email protected]>; Lazar, Lijo <[email protected]>; [email protected]
Subject: Re: FAILED: patch "[PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled" failed to apply to 5.14-stable tree
Quoted 9 lines · Greg KH
Quoted 8 lines · Michel Dänzer
Quoted 5 lines · On 2021-09-16 15:05, [email protected] wrote:
The patch below does not apply to the 5.14-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
It's already in 5.14, commit
32bc8f8373d2d6a681c96e4b25dca60d4d1c6016.
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
Odd, how were we supposed to know that?
Looks like the fix was merged separately for 5.14 and 5.15. I don't know how/why that happened. Alex / Christian?
The fix already landed in drm-next, but since this was before the merge window, we wanted to make sure the fix also landed in stable, so I cherry-picked it to 5.14. I'm not sure of a better way to handle these sort of cases.

Quoted 6 lines

You gotta give me a chance to know what happened. As the drm developers do this a lot, they have been putting the "cherry picked from" line in the patch because they can not merge between branches and keep the git id the same.
So try using the '-x' option to 'git cherry-pick' next time?

I can do that, but historically, I've gotten a lot of flack for that because the commit doesn't exist in Linus' tree yet at that point as the merge window hasn't opened yet.

Alex

Quoted 4 lines

thanks,
greg k-h

Greg KH

author maintainer

2021-09-16 17:44 UTC #7 lore raw

Quoted 53 lines · Alexander

[Public]
Quoted 51 lines
-----Original Message-----
From: Greg KH <[email protected]>
Sent: Thursday, September 16, 2021 11:36 AM
To: Deucher, Alexander <[email protected]>
Cc: Michel Dänzer <[email protected]>; Koenig, Christian <[email protected]>; Quan, Evan <[email protected]>; Lazar, Lijo <[email protected]>; [email protected]
Subject: Re: FAILED: patch "[PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled" failed to apply to 5.14-stable tree
Quoted 23 lines · Alexander
[Public]
Quoted 21 lines
-----Original Message-----
From: Michel Dänzer <[email protected]>
Sent: Thursday, September 16, 2021 9:55 AM
To: Greg KH <[email protected]>
Cc: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Quan, Evan <[email protected]>; Lazar, Lijo <[email protected]>; [email protected]
Subject: Re: FAILED: patch "[PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled" failed to apply to 5.14-stable tree
Quoted 9 lines · Greg KH
Quoted 8 lines · Michel Dänzer
Quoted 5 lines · On 2021-09-16 15:05, [email protected] wrote:
The patch below does not apply to the 5.14-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
It's already in 5.14, commit
32bc8f8373d2d6a681c96e4b25dca60d4d1c6016.
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
Odd, how were we supposed to know that?
Looks like the fix was merged separately for 5.14 and 5.15. I don't know how/why that happened. Alex / Christian?
The fix already landed in drm-next, but since this was before the merge window, we wanted to make sure the fix also landed in stable, so I cherry-picked it to 5.14. I'm not sure of a better way to handle these sort of cases.
You gotta give me a chance to know what happened. As the drm developers do this a lot, they have been putting the "cherry picked from" line in the patch because they can not merge between branches and keep the git id the same.

Quoted 5 lines

Quoted 1 line
So try using the '-x' option to 'git cherry-pick' next time?
I can do that, but historically, I've gotten a lot of flack for that because the commit doesn't exist in Linus' tree yet at that point as the merge window hasn't opened yet.

I totally agree, and would argue that this is NOT the way to do it either. But it seems like this is what the i915 developers have been doing for a few years now and honestly, it's better than nothing.

But I would like a real solution here, this gets very annoying every -rc1 cycle for the DRM subsystem. You all are the only ones with this problem for some strange reason :(

greg k-h