Re: [PATCH v2 09/10] drm/amdgpu: fix missing reset domain locks

[Christian König <christian.koenig@xxxxxxx>]

Am 31.05.24 um 00:02 schrieb Felix Kuehling:
> On 2024-05-28 13:23, Yunxiang Li wrote:
> > These functions are missing the lock for reset domain.
> >
> > Signed-off-by: Yunxiang Li <Yunxiang.Li@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c               | 4 +++-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c                | 8 ++++++--
> >   drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 9 +++++++--
> >   3 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> > index eb172388d99e..ddc5e9972da8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> > @@ -34,6 +34,7 @@
> >   #include <asm/set_memory.h>
> >   #endif
> >   #include "amdgpu.h"
> > +#include "amdgpu_reset.h"
> >   #include <drm/drm_drv.h>
> >   #include <drm/ttm/ttm_tt.h>
> >   @@ -401,13 +402,14 @@ void amdgpu_gart_invalidate_tlb(struct 
> > amdgpu_device *adev)
> >   {
> >       int i;
> >   -    if (!adev->gart.ptr)
> > +    if (!adev->gart.ptr || 
> > !down_read_trylock(&adev->reset_domain->sem))
> >           return;
> >         mb();
> >       amdgpu_device_flush_hdp(adev, NULL);
> >       for_each_set_bit(i, adev->vmhubs_mask, AMDGPU_MAX_VMHUBS)
> >           amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> > +    up_read(&adev->reset_domain->sem);
> >   }
> >     /**
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index e4742b65032d..52a3170d15b7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -307,8 +307,12 @@ static struct dma_fence *amdgpu_job_run(struct 
> > drm_sched_job *sched_job)
> >           dev_dbg(adev->dev, "Skip scheduling IBs in ring(%s)",
> >               ring->name);
> >       } else {
> > -        r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > -                       &fence);
> > +        r = -ETIME;
> > +        if (down_read_trylock(&adev->reset_domain->sem)) {
> > +            r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs,
> > +                           job, &fence);
> > +            up_read(&adev->reset_domain->sem);
> > +        }
> >           if (r)
> >               dev_err(adev->dev,
> >                   "Error scheduling IBs (%d) in ring(%s)", r,
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> > index 86ea610b16f3..21f5a1fb3bf8 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> > @@ -28,6 +28,7 @@
> >   #include "kfd_priv.h"
> >   #include "kfd_kernel_queue.h"
> >   #include "amdgpu_amdkfd.h"
> > +#include "amdgpu_reset.h"
> >     static inline struct process_queue_node *get_queue_by_qid(
> >               struct process_queue_manager *pqm, unsigned int qid)
> > @@ -87,8 +88,12 @@ void kfd_process_dequeue_from_device(struct 
> > kfd_process_device *pdd)
> >           return;
> >         dev->dqm->ops.process_termination(dev->dqm, &pdd->qpd);
> > -    if (dev->kfd->shared_resources.enable_mes)
> > -        amdgpu_mes_flush_shader_debugger(dev->adev, 
> > pdd->proc_ctx_gpu_addr);
> > +    if (dev->kfd->shared_resources.enable_mes &&
> > + down_read_trylock(&dev->adev->reset_domain->sem)) {
> > +        amdgpu_mes_flush_shader_debugger(dev->adev,
> > +                         pdd->proc_ctx_gpu_addr);
> > +
>
> It's not clear to me what's the requirement for reset domain locking 
> around MES calls. We have a lot more of them in 
> kfd_device_queue_manager.c (mostly calling adev->mes.funcs->... 
> directly). Do they all need to be wrapped individually?

Whenever you call a MES function (or any other function directly 
interacting with the rings or the HW registers) you need to make sure 
that at least the read side of the reset lock is held.

Regards,
Christian.

> Regards,
>   Felix
>
>
> > up_read(&dev->adev->reset_domain->sem);
> > +    }
> >       pdd->already_dequeued = true;
> >   }