Re: [PATCH 2/6] drm/amdgpu: add dedicated vmid field in vm struct

[zhoucm1 <david1.zhou-5C7GfCeVMHo@public.gmane.org>]

On 2017年04月26日 17:45, Christian König wrote:
> Am 26.04.2017 um 11:14 schrieb zhoucm1:
>>
>>
>> On 2017年04月26日 17:10, Christian König wrote:
>>> Am 26.04.2017 um 11:05 schrieb zhoucm1:
>>>>
>>>>
>>>> On 2017年04月26日 16:49, Christian König wrote:
>>>>> Am 26.04.2017 um 09:09 schrieb Zhang, Jerry (Junwei):
>>>>>> On 04/24/2017 01:57 PM, Chunming Zhou wrote:
>>>>>>> Change-Id: Id728e20366c8a1ae90d4e901dc80e136e2a613bb
>>>>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  2 ++
>>>>>>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> index eb429c5..acf9102 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> @@ -2144,10 +2144,12 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>>>>> *adev, struct amdgpu_vm *vm,
>>>>>>>       unsigned ring_instance;
>>>>>>>       struct amdgpu_ring *ring;
>>>>>>>       struct amd_sched_rq *rq;
>>>>>>> -    int r;
>>>>>>> +    int r, i;
>>>>>>>
>>>>>>>       vm->va = RB_ROOT;
>>>>>>>       vm->client_id = 
>>>>>>> atomic64_inc_return(&adev->vm_manager.client_counter);
>>>>>>> +    for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>>>>>>> +        vm->dedicated_vmid[i] = NULL;
>>>>>>
>>>>>> Maybe it's better to give it a consistent name as resv_vmid, or 
>>>>>> anything like that.
>>>>>
>>>>> Yes, agree.
>>>> I think the reserved vmid is dedicated to this vm, I don't know 
>>>> where this name doesn't make sense.
>>>>
>>>>> And if I'm not completely mistaken we still should only apply that 
>>>>> to the GFX hub on Vega10.
>>>> David Mao required mmhub as well. IIRC, we don't have necessary to 
>>>> argue more on this.
>>>
>>> I think we still have. There is no technical reason why we should 
>>> use the reserved/dedicated VM for other engines than the one 
>>> involved in the SQ trace.
>> How about removing SQ trace reason? is it ok just that one process 
>> wants to do experiment for other purpose? :)
>
> In this case I would clearly NAK the whole approach.

Which makes me very difficult to talk between you and David Mao.

Regards,
David Zhou
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>
>>> So I would say we should limit this to GFX and Compute jobs and only 
>>> allocate the dedicated VMID for those.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> David Zhou
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Jerry
>>>>>>> spin_lock_init(&vm->status_lock);
>>>>>>>       INIT_LIST_HEAD(&vm->invalidated);
>>>>>>>       INIT_LIST_HEAD(&vm->cleared);
>>>>>>> @@ -2250,6 +2252,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;
>>>>>>> +    int i;
>>>>>>>
>>>>>>>       if (vm->is_kfd_vm) {
>>>>>>>           struct amdgpu_vm_id_manager *id_mgr =
>>>>>>> @@ -2292,6 +2295,18 @@ void amdgpu_vm_fini(struct amdgpu_device 
>>>>>>> *adev, struct amdgpu_vm *vm)
>>>>>>>
>>>>>>>       amdgpu_vm_free_levels(&vm->root);
>>>>>>>       fence_put(vm->last_dir_update);
>>>>>>> +    for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
>>>>>>> +        struct amdgpu_vm_id_manager *id_mgr =
>>>>>>> +            &adev->vm_manager.id_mgr[i];
>>>>>>> +
>>>>>>> +        mutex_lock(&id_mgr->lock);
>>>>>>> +        if (vm->dedicated_vmid[i]) {
>>>>>>> + list_add(&vm->dedicated_vmid[i]->list,
>>>>>>> +                 &id_mgr->ids_lru);
>>>>>>> +            vm->dedicated_vmid[i] = NULL;
>>>>>>> +        }
>>>>>>> +        mutex_unlock(&id_mgr->lock);
>>>>>>> +    }
>>>>>>>   }
>>>>>>>
>>>>>>>   /**
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>>> index 62dbace..23981ee 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>>> @@ -122,6 +122,8 @@ struct amdgpu_vm {
>>>>>>>
>>>>>>>       /* client id */
>>>>>>>       u64                     client_id;
>>>>>>> +    /* dedicated vmid */
>>>>>>> +    struct amdgpu_vm_id *dedicated_vmid[AMDGPU_MAX_VMHUBS];
>>>>>>>       /* each VM will map on CSA */
>>>>>>>       struct amdgpu_bo_va *csa_bo_va;
>>>>>>>
>>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx