Re: [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 
.


Patch #3:

+/* Select the first entry VMID */
+idle = list_first_entry(_mgr->ids_lru, struct amdgpu_vm_id, 
list);

+list_del_init(>list);
+vm->reserved_vmid[vmhub] = idle;
+mutex_unlock(_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(>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(>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(>gpu_reset_counter);

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

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

[zhoucm1]

On 2017年04月27日 17:51, Christian König wrote:
Patch #1, #2, #4 and #6 are Reviewed-by: Christian König 
.


Patch #3:

+/* Select the first entry VMID */
+idle = list_first_entry(_mgr->ids_lru, struct amdgpu_vm_id, 
list);

+list_del_init(>list);
+vm->reserved_vmid[vmhub] = idle;
+mutex_unlock(_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(>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(>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(>gpu_reset_counter);

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

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

[Christian König]

Patch #1, #2, #4 and #6 are Reviewed-by: Christian König 
.


Patch #3:

+   /* Select the first entry VMID */
+   idle = list_first_entry(_mgr->ids_lru, struct amdgpu_vm_id, list);
+   list_del_init(>list);
+   vm->reserved_vmid[vmhub] = idle;
+   mutex_unlock(_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.


Patch #5:

+   if ((amdgpu_vm_had_gpu_reset(adev, id)) ||
+   (atomic64_read(>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)))


+   tmp = amdgpu_sync_get_fence(>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.


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.



+   id->pd_gpu_addr = job->vm_pd_addr;
+   id->current_gpu_reset_count = atomic_read(>gpu_reset_counter);
+   atomic64_set(>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;

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

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

[Zhang, Jerry (Junwei)]

It looks fine for me now.

Reviewed-by: Junwei Zhang 

Jerry

On 04/27/2017 01:00 PM, Chunming Zhou wrote:

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

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

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

-- 
1.9.1

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