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

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

On 2017年04月27日 17:51, Christian König wrote:
> Patch #1, #2, #4 and #6 are Reviewed-by: Christian König 
> <christian.koenig@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?

>
>> +    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(-)
>>
>

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