[PATCH 4/6] drm/amdgpu: add limitation for dedicated vm number v2

[Christian König]

Am 26.04.2017 um 09:10 schrieb Zhang, Jerry (Junwei):
> On 04/24/2017 01:57 PM, Chunming Zhou wrote:
>> v2: move #define to amdgpu_vm.h
>>
>> Change-Id: Ie5958cf6dbdc1c8278e61d9158483472d6f5c6e3
>> Signed-off-by: Chunming Zhou <David1.Zhou at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 9 +++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     | 2 ++
>>   4 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 0831cd2..ba9d3d0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1591,6 +1591,7 @@ struct amdgpu_device {
>>       struct amdgpu_dummy_page    dummy_page;
>>       struct amdgpu_vm_manager    vm_manager;
>>       struct amdgpu_vmhub             vmhub[AMDGPU_MAX_VMHUBS];
>> +    atomic_t            reserved_vmid;
>
> Perhaps it's more reasonable to belongs to vm_manager.

Yes, agree.

>
> Jerry
>
>>
>>       /* memory management */
>>       struct amdgpu_mman        mman;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a175dfd..9993085 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1889,6 +1889,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>       adev->vm_manager.vm_pte_num_rings = 0;
>>       adev->gart.gart_funcs = NULL;
>>       adev->fence_context = kcl_fence_context_alloc(AMDGPU_MAX_RINGS);
>> +    atomic_set(&adev->reserved_vmid, 0);
>>
>>       adev->smc_rreg = &amdgpu_invalid_rreg;
>>       adev->smc_wreg = &amdgpu_invalid_wreg;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 5f4dcc9..f7113b9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -565,6 +565,10 @@ static int amdgpu_vm_alloc_dedicated_vmid(struct 
>> amdgpu_device *adev,
>>       unsigned vmhub;
>>       int r;
>>
>> +    if (atomic_read(&adev->reserved_vmid) >= 
>> AMDGPU_VM_MAX_RESERVED_VMID) {
>> +        DRM_ERROR("Over limitation of reserved vmid\n");
>> +        return -EINVAL;
>> +    }
>>       for (vmhub = 0; vmhub < AMDGPU_MAX_VMHUBS; vmhub++) {
>>           id_mgr = &adev->vm_manager.id_mgr[vmhub];
>>
>> @@ -580,6 +584,7 @@ static int amdgpu_vm_alloc_dedicated_vmid(struct 
>> amdgpu_device *adev,
>>           if (r)
>>               goto err;
>>       }
>> +    atomic_inc(&adev->reserved_vmid);

That's not atomic at all, two threads could try to reserve a VMID at the 
same time and race.

You need to use atomic_inc_return for this before allocating the IDs and 
properly clean up if allocating them fails.

>>
>>       return 0;
>>   err:
>> @@ -2302,6 +2307,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm)
>>   {
>>       struct amdgpu_bo_va_mapping *mapping, *tmp;
>>       bool prt_fini_needed = !!adev->gart.gart_funcs->set_prt;
>> +    bool dedicated = false;
>>       int i;
>>
>>       if (vm->is_kfd_vm) {
>> @@ -2354,9 +2360,12 @@ void amdgpu_vm_fini(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm)
>>               list_add(&vm->dedicated_vmid[i]->list,
>>                    &id_mgr->ids_lru);
>>               vm->dedicated_vmid[i] = NULL;
>> +            dedicated = true;
>>           }
>>           mutex_unlock(&id_mgr->lock);
>>       }
>> +    if (dedicated)
>> +        atomic_dec(&adev->reserved_vmid);
>>   }
>>
>>   /**
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 23981ee..2d3e6ed 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -83,6 +83,8 @@
>>
>>   /* hardcode that limit for now */
>>   #define AMDGPU_VA_RESERVED_SIZE            (8 << 20)
>> +/* max vmids dedicated for process */
>> +#define AMDGPU_VM_MAX_RESERVED_VMID 2

This should be 1 instead of 2, or did I miss something?

Regards,
Christian.

>>
>>   struct amdgpu_vm_pt {
>>       struct amdgpu_bo    *bo;
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx