[PATCH 0/6 v4] *** Dedicated vmid per process v4 ***

[Christian König]

Am 27.04.2017 um 12:26 schrieb zhoucm1:
>
>
> On 2017年04月27日 17:51, Christian König wrote:
>> Patch #1, #2, #4 and #6 are Reviewed-by: Christian König 
>> <christian.koenig at amd.com>.
>>
>> Patch #3:
>>> +    /* Select the first entry VMID */
>>> +    idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id, 
>>> list);
>>> +    list_del_init(&idle->list);
>>> +    vm->reserved_vmid[vmhub] = idle;
>>> +    mutex_unlock(&id_mgr->lock);
>> I think we should wait for the VMID to be completely idle before we 
>> can use it, but that possible also go into the handling in patch #5.
> Yes.
>>
>> Patch #5:
>>> +    if ((amdgpu_vm_had_gpu_reset(adev, id)) ||
>>> +        (atomic64_read(&id->owner) != vm->client_id) ||
>>> +        (job->vm_pd_addr != id->pd_gpu_addr) ||
>>> +        (updates && (!flushed || updates->context != 
>>> flushed->context ||
>>> +            fence_is_later(updates, flushed)))) {
>> You also need this check here as well:
>>                 if (!id->last_flush ||
>>                     (id->last_flush->context != fence_context &&
>>                      !fence_is_signaled(id->last_flush)))
> added.
>>
>>> +        tmp = amdgpu_sync_get_fence(&id->active);
>>> +        if (tmp) {
>>> +            r = amdgpu_sync_fence(adev, sync, tmp);
>>> +            fence_put(tmp);
>>> +            return r;
>>> +        }
>> That won't work correctly.
>>
>> The first problem is that amdgpu_sync_get_fence() removes the fence 
>> from the active fences. So a another command submission from a 
>> different context won't wait for all the necessary fences. I think 
>> using amdgpu_sync_peek_fence() instead should work here.
> good catch.
>
>>
>> The second problem is that a context could be starved when it needs a 
>> VM flush while another context can still submit jobs without a flush. 
>> I think you could work around that by setting id->pd_gpu_addr to an 
>> invalid value when we hit this case. This way all other contexts 
>> would need to do a VM flush as well.
> I don't catch your opinion, when you object concurrent flush, isn't it 
> expected?

Imagine the following scenario:

Two contexts in use by the application.

Context 1 has a bunch of jobs to do, but needs a VM flush before 
starting them.

So each job will just pick the first fence to wait for go to sleep again.

Context 2 has a bunch of jobs to do, but does NOT need a VM flush.

This will add more and more fences to the collection of active jobs for 
this id.

The result is that Context 1 will never be able to submit any of it's 
jobs because context 2 keeps the ID busy all the time.

Setting the pd_gpu_addr to some invalid value (or maybe the ID owner?) 
should fix that. In this case context 2 needs to flush as well and so 
context 1 will sooner or later get a chance as well.

Regards,
Christian.

>
>
>>
>>> +    id->pd_gpu_addr = job->vm_pd_addr;
>>> +    id->current_gpu_reset_count = 
>>> atomic_read(&adev->gpu_reset_counter);
>>> +    atomic64_set(&id->owner, vm->client_id);
>>> +    job->vm_needs_flush = needs_flush;
>> If we need a flush id->last_flush needs to be set to NULL here as 
>> well. E.g. do
>>         fence_put(id->last_flush);
>>         id->last_flush = NULL;
> changed.
>
> Regards,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>> Am 27.04.2017 um 07:00 schrieb Chunming Zhou:
>>> The current kernel implementation, which grabs the idle VMID from 
>>> pool when emitting the job may:
>>>
>>>      The back-to-back submission from one process could use 
>>> different VMID.
>>>      The submission to different queues from single process could 
>>> use different VMID
>>>
>>> It works well in most case but cannot work for the SQ thread trace 
>>> capture.
>>>
>>> The VMID for the submission that set the {SQTT}_BASE, which refers 
>>> to the address of the trace buffer, is stored in shader engine.
>>>
>>> If the profiling application have to use different VMIDs to submit 
>>> IBs in its life cycle:
>>>
>>>      Some trace is not captured since it actually uses different 
>>> VMID to submit jobs.
>>>      Some part of captured trace may come from different application 
>>> since they are accidentally uses the owner’s VMID to submit jobs.
>>>
>>> V2:
>>> 1. address Christian's comments:
>>>     a. drop context flags for tag process, instead, add vm ioctl.
>>>     b. change order of patches.
>>>     c. sync waiting only when vm flush needs.
>>>
>>> 2. address Alex's comments;
>>>     bump module version
>>>
>>> V3:
>>>    address Jerry and Christian's comments.
>>>    and only reserve gfxhub vmid
>>>
>>> v4:
>>>    address Jerry and Christian's comments.
>>>    fix some race condistions.
>>>
>>> Chunming Zhou (6):
>>>    drm/amdgpu: add vm ioctl
>>>    drm/amdgpu: add reserved vmid field in vm struct v2
>>>    drm/amdgpu: reserve/unreserve vmid by vm ioctl v4
>>>    drm/amdgpu: add limitation for dedicated vm number v4
>>>    drm/amdgpu: implement grab reserved vmid V3
>>>    drm/amdgpu: bump module verion for reserved vmid
>>>
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |   3 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |   1 +
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 152 
>>> ++++++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |   6 ++
>>>   include/uapi/drm/amdgpu_drm.h           |  22 +++++
>>>   5 files changed, 178 insertions(+), 6 deletions(-)
>>>
>>
>