Applied. Thanks!
Alex
On Mon, Aug 16, 2021 at 11:07 AM Michel Dänzer michel@daenzer.net wrote:
On 2021-08-16 2:06 p.m., Christian König wrote:
Am 16.08.21 um 13:33 schrieb Lazar, Lijo:
On 8/16/2021 4:05 PM, Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
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)
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer mdaenzer@redhat.com
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 +++++------ drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 22 +++++++++++++++++----- 2 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index f3fd5ec710b6..f944ed858f3e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -2777,12 +2777,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);
Don't see any case for this. It's not expected to be scheduled in this case, right?
- WARN_ON_ONCE(adev->gfx.gfx_off_req_count);
Thinking about ON_ONCE here - this may happen more than once if it's completed as part of cancel_ call. Is the warning needed?
WARN_ON_ONCE() is usually used to prevent spamming the system log with warnings. E.g. the warning is only printed once indicating a driver bug and that's it.
Right, these WARN_ONs are like assert()s in user-space code, documenting the pre-conditions and checking them at runtime. And I use _ONCE so that if a pre-condition is ever violated for some reason, dmesg isn't spammed with multiple warnings.
Anyway, Reviewed-by: Lijo Lazar lijo.lazar@amd.com
Acked-by: Christian König christian.koenig@amd.com
Thanks guys!
-- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer