| Message ID | 20220620220302.86389-4-andrey.grodzovsky@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework amdgpu HW fence refocunt and update scheduler parent fence refcount. | expand |
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@amd.com> > --- > 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. > + } > + > + /* 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 :) 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.
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@amd.com> >> --- >> 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. >
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@amd.com> >>> --- >>> 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. >>
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@amd.com> >>>> --- >>>> 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.
Just a ping Andrey On 2022-06-21 15:45, Andrey Grodzovsky wrote: > > 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@amd.com> >>> --- >>> 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. >>
Am 22.06.22 um 19:31 schrieb Andrey Grodzovsky: > Just a ping You need to give me at least some time to look into this. > > Andrey > > On 2022-06-21 15:45, Andrey Grodzovsky wrote: >> >> 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@amd.com> >>>> --- >>>> 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'm not 100% sure of that. The original idea was to enable/disable interrupt sources as they came along. >> I can disable amdgpu interrupt line totally using disable_irq - would >> this be better ? Yes, that's what I thought as well. >> >> >>> >>> >>>> + } >>>> + >>>> + /* 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. That should be sufficient. When a local interrupt would run the code here wouldn't be executing. >> I guess we can also introduce a spinlock to serialize them ? Yiqing >> reported seeing a race between them so we have to do something. A spinlock would be an absolute NAK because we have been working quite hard to remove them (for multiple reasons). Christian. >> >> 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. >>>
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); + } + + /* TODO Only waits for irq handlers on other CPUs, maybe local_irq_save + * local_irq_local_irq_restore are needed here for local interrupts ? + * + */ + 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.
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@amd.com> --- 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(+)