You have a job in the pending list which is marked as not finished in
drm_sched_stop
(https://elixir.bootlin.com/linux/v5.16/source/drivers/gpu/drm/scheduler/sched_main.c#L420),
s_fence signal cb removed and the job is kept in pending list.
Later you will try to manually clear the HW fence of this job in here
(https://elixir.bootlin.com/linux/v5.16/source/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c#L4492)
but the EOP interrupt can fire for that fence exactly at this
moment and you have concurrent access to fence driver's fence array
from both amdgpu_process_fence and amdgpu_fence_driver_clear_job_fences
which is not supposed to happen.Yiqing reported to me a race during
debugging
we did of the original refcount bug and it looked to me like this
scenario. Seems to me the EOP ISR handler should be prevented from
running during this time at least.
Andrey
On 2022-06-21 21:47, VURDIGERENATARAJ, CHANDAN wrote:
Hi,
Is this a preventive fix or you found errors/oops/hangs?
If you had found errors/oops/hangs, can you please share the details?
BR,
Chandan V N
On 2022-06-21 03:25, Christian König wrote:
Am 21.06.22 um 00:03 schrieb Andrey Grodzovsky:
Problem:
After we start handling timed out jobs we assume there fences won't
be signaled but we cannot be sure and sometimes they fire late. We
need to prevent concurrent accesses to fence array from
amdgpu_fence_driver_clear_job_fences during GPU reset and
amdgpu_fence_process from a late EOP interrupt.
Fix:
Before accessing fence array in GPU disable EOP interrupt and flush
all pending interrupt handlers for amdgpu device's interrupt line.
Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++
drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 26
++++++++++++++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
3 files changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 2b92281dd0c1..c99541685804 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4605,6 +4605,8 @@ int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
amdgpu_virt_fini_data_exchange(adev);
}
+ amdgpu_fence_driver_isr_toggle(adev, true);
+
/* block all schedulers and reset given job's ring */
for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
struct amdgpu_ring *ring = adev->rings[i]; @@ -4620,6
+4622,8 @@ int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
amdgpu_fence_driver_force_completion(ring);
}
+ amdgpu_fence_driver_isr_toggle(adev, false);
+
if (job && job->vm)
drm_sched_increase_karma(&job->base);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index a9ae3beaa1d3..d6d54ba4c185 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -532,6 +532,32 @@ void amdgpu_fence_driver_hw_fini(struct
amdgpu_device *adev)
}
}
+void amdgpu_fence_driver_isr_toggle(struct amdgpu_device *adev,
bool stop)
+{
+ int i;
+
+ for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
+ struct amdgpu_ring *ring = adev->rings[i];
+
+ if (!ring || !ring->fence_drv.initialized ||
!ring->fence_drv.irq_src)
+ continue;
+
+ if (stop)
+ amdgpu_irq_put(adev, ring->fence_drv.irq_src,
+ ring->fence_drv.irq_type);
+ else
+ amdgpu_irq_get(adev, ring->fence_drv.irq_src,
+ ring->fence_drv.irq_type);
That won't work like this. This increments/decrements the reference
count for the IRQ, but doesn't guarantee in any way that they are
stopped/started.
I understand that, i just assumed that the fence driver is the only holder of this interrupt source (e.g. regCP_INT_CNTL_RING0) ?
I can disable amdgpu interrupt line totally using disable_irq - would this be better ?
+ }
+
+ /* TODO Only waits for irq handlers on other CPUs, maybe
local_irq_save
+ * local_irq_local_irq_restore are needed here for local
interrupts ?
+ *
+ */
Well that comment made me smile. Think for a moment what the local CPU
would be doing if an interrupt would run :)
No, I understand this of course, I am ok to be interrupted by interrupt handler at this point, what i am trying to do is to prevent amdgpu_fence_process to run concurrently with amdgpu_fence_driver_clear_job_fences - that is what this function is trying to prevent - i disable and >flush pending EOP ISR handlers before the call to clear fences and re-enable after.
I guess we can also introduce a spinlock to serialize them ? Yiqing reported seeing a race between them so we have to do something.
Andrey
Cheers,
Christian.
+ if (stop)
+ synchronize_irq(adev->irq.irq); }
+
void amdgpu_fence_driver_sw_fini(struct amdgpu_device *adev)
{
unsigned int i, j;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 7d89a52091c0..82c178a9033a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -143,6 +143,7 @@ signed long amdgpu_fence_wait_polling(struct
amdgpu_ring *ring,
uint32_t wait_seq,
signed long timeout);
unsigned amdgpu_fence_count_emitted(struct amdgpu_ring *ring);
+void amdgpu_fence_driver_isr_toggle(struct amdgpu_device *adev, bool
stop);
/*
* Rings.
