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