Re: [PATCH 3/5] drm/amdgpu: Prevent race between late signaled fences and GPU reset.

[Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>]

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.