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

[Felix Kuehling <felix.kuehling@amd.com>]

On 2024-05-31 2:52, Christian König wrote:
> 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@amd.com>
>>> ---
>>>   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.

Having to do that for each caller of amdgpu_mes functions seems error prone.

Would it make sense to wrap that inside amdgpu_mes_lock/unlock? Maybe turn it into amdgpu_mes_trylock/unlock and make sure that all the amdgpu_mes functions that take that lock can fail and return an error code. Add an attribute so the compiler can flag callers that ignore the return values. This would make it easier to let the compiler spot places that don't handle errors due to reset lock failures.

Regards,
  Felix

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