Re: [PATCH 3/3] drm/amdgpu: Do not fiddle with the idle workers too much

[Tvrtko Ursulin]

On 26/06/2026 19:59, Tvrtko Ursulin wrote:

On 26/06/2026 18:15, Timur Kristóf wrote:
On 2026. június 26., péntek 10:55:58 közép-európai nyári idő Tvrtko 
Ursulin
wrote:
Idle workers only need to be canceled or pushed back if we are 
potentially
idle. Make the both operations conditional on the pre-increment and 
post-
decrement status of the in-flight job counter.


Nice catch!

I now have some second thoughts about this one. Think I have inverted 
the logic of what it needs to achieve. I blame the heat wave :) but at 
least I am pretty sure there is still a way to make it more efficient. I 
will re-visit next week.

Okay I now again think the patch is fine. I got confused by the 
total_submission_count tracking only in flight ring emissions, while the 
idle worker has a secondary gate on emitted fences.

I had some ideas on how to further optimise this, by reducing to a 
single criteria, moving the counter tracking to track hw fences emission 
and retire, and also to prefer just letting the idle worker fire instead 
of canceling (much cheaper to self-cancel), but.. I got bogged down in 
the VCN code and did not get it all working in reasonable time.

So I think for now if AMD is happy with this one, I think it could be 
picked up as is, and I can follow up with further improvements when I 
get some more free time.

Regards,

Tvrtko


Reviewed-by: Timur Kristóf <timur.kris...@gmail.com>

Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Cc: Christian König <christian.koe...@amd.com>
Cc: Timur Kristóf <timur.kris...@gmail.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c  | 11 +++++------
  drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c |  9 +++++----
  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c  | 12 +++++-------
  drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c    | 12 +++++-------
  4 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c index 
85372af1216d..623a5339bc47
100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -2460,9 +2460,8 @@ void amdgpu_gfx_profile_ring_begin_use(struct
amdgpu_ring *ring) else
          profile = PP_SMC_POWER_PROFILE_COMPUTE;

-    atomic_inc(&adev->gfx.total_submission_cnt);
-
-    cancel_delayed_work_sync(&adev->gfx.idle_work);
+    if (!atomic_fetch_inc(&adev->gfx.total_submission_cnt))
+        cancel_delayed_work_sync(&adev->gfx.idle_work);

      /* We can safely return early here because we've cancelled the
       * the delayed work so there is no one else to set it to false
@@ -2490,9 +2489,9 @@ void amdgpu_gfx_profile_ring_end_use(struct
amdgpu_ring *ring) if (amdgpu_dpm_is_overdrive_enabled(adev))
          return;

-    atomic_dec(&ring->adev->gfx.total_submission_cnt);
-
-    schedule_delayed_work(&ring->adev->gfx.idle_work,
GFX_PROFILE_IDLE_TIMEOUT); +    if
(atomic_dec_and_test(&ring->adev->gfx.total_submission_cnt))
+        schedule_delayed_work(&ring->adev->gfx.idle_work,
+                      GFX_PROFILE_IDLE_TIMEOUT);
  }

  /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c index 
63ee6ba6a931..57935c321515
100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
@@ -134,8 +134,8 @@ void amdgpu_jpeg_ring_begin_use(struct amdgpu_ring
*ring) {
      struct amdgpu_device *adev = ring->adev;

-    atomic_inc(&adev->jpeg.total_submission_cnt);
-    cancel_delayed_work_sync(&adev->jpeg.idle_work);
+    if (!atomic_fetch_inc(&adev->jpeg.total_submission_cnt))
+        cancel_delayed_work_sync(&adev->jpeg.idle_work);

      mutex_lock(&adev->jpeg.jpeg_pg_lock);
      amdgpu_device_ip_set_powergating_state(adev,
AMD_IP_BLOCK_TYPE_JPEG,
@@ -145,8 +145,9 @@ void amdgpu_jpeg_ring_begin_use(struct amdgpu_ring
*ring)

  void amdgpu_jpeg_ring_end_use(struct amdgpu_ring *ring)
  {
-    atomic_dec(&ring->adev->jpeg.total_submission_cnt);
-    schedule_delayed_work(&ring->adev->jpeg.idle_work,
JPEG_IDLE_TIMEOUT);
+    if (atomic_dec_and_test(&ring->adev->jpeg.total_submission_cnt))
+        schedule_delayed_work(&ring->adev->jpeg.idle_work,
+                      JPEG_IDLE_TIMEOUT);
  }

  int amdgpu_jpeg_dec_ring_test_ring(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c index 
b261aa7c1ba8..8d2abf706dfd
100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -506,9 +506,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring 
*ring)
struct amdgpu_device *adev = ring->adev;
      struct amdgpu_vcn_inst *vcn_inst = &adev->vcn.inst[ring->me];

-    atomic_inc(&vcn_inst->total_submission_cnt);
-
-    cancel_delayed_work_sync(&vcn_inst->idle_work);
+    if (!atomic_fetch_inc(&vcn_inst->total_submission_cnt))
+        cancel_delayed_work_sync(&vcn_inst->idle_work);

      mutex_lock(&vcn_inst->vcn_pg_lock);
      vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
@@ -550,10 +549,9 @@ void amdgpu_vcn_ring_end_use(struct amdgpu_ring 
*ring)
          !adev->vcn.inst[ring->me].using_unified_queue)
          atomic_dec(&ring->adev->vcn.inst[ring-
me].dpg_enc_submission_cnt);

-    atomic_dec(&ring->adev->vcn.inst[ring->me].total_submission_cnt);
-
-    schedule_delayed_work(&ring->adev->vcn.inst[ring->me].idle_work,
-                  VCN_IDLE_TIMEOUT);
+    if
(atomic_dec_and_test(&ring->adev->vcn.inst[ring- 
>me].total_submission_cnt))
+        schedule_delayed_work(&ring->adev->vcn.inst[ring-
me].idle_work, +
    VCN_IDLE_TIMEOUT);
  }

  int amdgpu_vcn_dec_ring_test_ring(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c index 8b8184fe6764..0d8a3cea63ee
100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
@@ -159,9 +159,8 @@ static void vcn_v2_5_ring_begin_use(struct 
amdgpu_ring
*ring) struct amdgpu_device *adev = ring->adev;
      struct amdgpu_vcn_inst *v = &adev->vcn.inst[ring->me];

-    atomic_inc(&adev->vcn.inst[0].total_submission_cnt);
-
-    cancel_delayed_work_sync(&adev->vcn.inst[0].idle_work);
+    if (!atomic_fetch_inc(&adev->vcn.inst[0].total_submission_cnt))
+        cancel_delayed_work_sync(&adev->vcn.inst[0].idle_work);

      /* We can safely return early here because we've cancelled the
       * the delayed work so there is no one else to set it to false
@@ -207,10 +206,9 @@ static void vcn_v2_5_ring_end_use(struct 
amdgpu_ring
*ring) !adev->vcn.inst[ring->me].using_unified_queue)
          atomic_dec(&adev->vcn.inst[ring-
me].dpg_enc_submission_cnt);

-    atomic_dec(&adev->vcn.inst[0].total_submission_cnt);
-
-    schedule_delayed_work(&adev->vcn.inst[0].idle_work,
-                  VCN_IDLE_TIMEOUT);
+    if (atomic_dec_and_test(&adev->vcn.inst[0].total_submission_cnt))
+        schedule_delayed_work(&adev->vcn.inst[0].idle_work,
+                      VCN_IDLE_TIMEOUT);
  }

  /**