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

[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

[PATCH 1/6] drm/amdgpu: add vm ioctl

[Chunming Zhou]

It will be used for reserving vmid.

Change-Id: Ib7169ea999690c8e82d0dcbccdd2d97760c0270a
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 18 ++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  1 +
 include/uapi/drm/amdgpu_drm.h           | 22 ++++++++++++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 052e6a5..8ea33c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1062,6 +1062,7 @@ int amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
 const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {
 	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_CREATE, amdgpu_gem_create_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(AMDGPU_CTX, amdgpu_ctx_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(AMDGPU_VM, amdgpu_vm_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(AMDGPU_BO_LIST, amdgpu_bo_list_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
 	/* KMS */
 	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_MMAP, amdgpu_gem_mmap_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a8cd23b..8698177 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2379,3 +2379,21 @@ void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
 		}
 	}
 }
+
+int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
+{
+	union drm_amdgpu_vm *args = data;
+	struct amdgpu_device *adev = dev->dev_private;
+	struct amdgpu_fpriv *fpriv = filp->driver_priv;
+
+	switch (args->in.op) {
+	case AMDGPU_VM_OP_RESERVE_VMID:
+	case AMDGPU_VM_OP_UNRESERVE_VMID:
+		return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 8eba2d3..16e0aaa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -248,5 +248,6 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
 void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 		      struct amdgpu_bo_va *bo_va);
 void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint64_t vm_size);
+int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
 
 #endif
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index b98b371..1c95775 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -51,6 +51,7 @@
 #define DRM_AMDGPU_GEM_OP		0x10
 #define DRM_AMDGPU_GEM_USERPTR		0x11
 #define DRM_AMDGPU_WAIT_FENCES		0x12
+#define DRM_AMDGPU_VM			0x13
 
 /* hybrid specific ioctls */
 #define DRM_AMDGPU_SEM			0x5b
@@ -71,6 +72,7 @@
 #define DRM_IOCTL_AMDGPU_GEM_OP		DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_OP, struct drm_amdgpu_gem_op)
 #define DRM_IOCTL_AMDGPU_GEM_USERPTR	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_USERPTR, struct drm_amdgpu_gem_userptr)
 #define DRM_IOCTL_AMDGPU_WAIT_FENCES	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_WAIT_FENCES, union drm_amdgpu_wait_fences)
+#define DRM_IOCTL_AMDGPU_VM		DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_VM, union drm_amdgpu_vm)
 
 /* hybrid specific ioctls */
 #define DRM_IOCTL_AMDGPU_GEM_DGMA	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_DGMA, struct drm_amdgpu_gem_dgma)
@@ -212,6 +214,26 @@ struct drm_amdgpu_ctx_in {
 	union drm_amdgpu_ctx_out out;
 };
 
+/* vm ioctl */
+#define AMDGPU_VM_OP_RESERVE_VMID	1
+#define AMDGPU_VM_OP_UNRESERVE_VMID	2
+
+struct drm_amdgpu_vm_in {
+	/** AMDGPU_VM_OP_* */
+	__u32	op;
+	__u32	flags;
+};
+
+struct drm_amdgpu_vm_out {
+	/** For future use, no flags defined so far */
+	__u64	flags;
+};
+
+union drm_amdgpu_vm {
+	struct drm_amdgpu_vm_in in;
+	struct drm_amdgpu_vm_out out;
+};
+
 /* sem related */
 #define AMDGPU_SEM_OP_CREATE_SEM        1
 #define AMDGPU_SEM_OP_WAIT_SEM	        2
-- 
1.9.1

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

[PATCH 2/6] drm/amdgpu: add reserved vmid field in vm struct v2

[Chunming Zhou]

v2: rename dedicated_vmid to reserved_vmid

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 8698177..261aaea 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2163,10 +2163,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->reserved_vmid[i] = NULL;
 	spin_lock_init(&vm->status_lock);
 	INIT_LIST_HEAD(&vm->invalidated);
 	INIT_LIST_HEAD(&vm->cleared);
@@ -2270,6 +2272,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 =
@@ -2313,6 +2316,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->reserved_vmid[i]) {
+			list_add(&vm->reserved_vmid[i]->list,
+				 &id_mgr->ids_lru);
+			vm->reserved_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 16e0aaa..8eedca1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -123,6 +123,8 @@ struct amdgpu_vm {
 
 	/* client id */
 	u64                     client_id;
+	/* dedicated to vm */
+	struct amdgpu_vm_id	*reserved_vmid[AMDGPU_MAX_VMHUBS];
 	/* each VM will map on CSA */
 	struct amdgpu_bo_va *csa_bo_va;
 
-- 
1.9.1

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

[PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v4

[Chunming Zhou]

add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
v4:
fix racy condition

Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 62 +++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 261aaea..e37421e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -546,6 +546,45 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 	return r;
 }
 
+static void amdgpu_vm_free_reserved_vmid(struct amdgpu_device *adev,
+					  struct amdgpu_vm *vm,
+					  unsigned vmhub)
+{
+	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
+
+	mutex_lock(&id_mgr->lock);
+	if (vm->reserved_vmid[vmhub]) {
+		list_add(&vm->reserved_vmid[vmhub]->list,
+			&id_mgr->ids_lru);
+		vm->reserved_vmid[vmhub] = NULL;
+	}
+	mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_reserved_vmid(struct amdgpu_device *adev,
+					 struct amdgpu_vm *vm,
+					 unsigned vmhub)
+{
+	struct amdgpu_vm_id_manager *id_mgr;
+	struct amdgpu_vm_id *idle;
+	int r = 0;
+
+	id_mgr = &adev->vm_manager.id_mgr[vmhub];
+	mutex_lock(&id_mgr->lock);
+	if (vm->reserved_vmid[vmhub])
+		goto unlock;
+	/* 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);
+
+	return 0;
+unlock:
+	mutex_unlock(&id_mgr->lock);
+	return r;
+}
+
 static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
 {
 	struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2355,8 @@ 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->reserved_vmid[i]) {
-			list_add(&vm->reserved_vmid[i]->list,
-				 &id_mgr->ids_lru);
-			vm->reserved_vmid[i] = NULL;
-		}
-		mutex_unlock(&id_mgr->lock);
-	}
+	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+		amdgpu_vm_free_reserved_vmid(adev, vm, i);
 }
 
 /**
@@ -2400,11 +2429,18 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 	union drm_amdgpu_vm *args = data;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_fpriv *fpriv = filp->driver_priv;
+	int r;
 
 	switch (args->in.op) {
 	case AMDGPU_VM_OP_RESERVE_VMID:
+		/* current, we only have requirement to reserve vmid from gfxhub */
+		r = amdgpu_vm_alloc_reserved_vmid(adev, &fpriv->vm,
+						  AMDGPU_GFXHUB);
+		if (r)
+			return r;
+		break;
 	case AMDGPU_VM_OP_UNRESERVE_VMID:
-		return -EINVAL;
+		amdgpu_vm_free_reserved_vmid(adev, &fpriv->vm, AMDGPU_GFXHUB);
 		break;
 	default:
 		return -EINVAL;
-- 
1.9.1

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

Re: [PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v4

[Zhang, Jerry (Junwei)]

On 04/27/2017 01:00 PM, Chunming Zhou wrote:
> add reserve/unreserve vmid funtions.
> v3:
> only reserve vmid from gfxhub
> v4:
> fix racy condition
>
> Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 62 +++++++++++++++++++++++++++-------
>   1 file changed, 49 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 261aaea..e37421e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -546,6 +546,45 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   	return r;
>   }
>
> +static void amdgpu_vm_free_reserved_vmid(struct amdgpu_device *adev,
> +					  struct amdgpu_vm *vm,
> +					  unsigned vmhub)
> +{
> +	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> +
> +	mutex_lock(&id_mgr->lock);
> +	if (vm->reserved_vmid[vmhub]) {
> +		list_add(&vm->reserved_vmid[vmhub]->list,
> +			&id_mgr->ids_lru);
> +		vm->reserved_vmid[vmhub] = NULL;
> +	}
> +	mutex_unlock(&id_mgr->lock);
> +}
> +
> +static int amdgpu_vm_alloc_reserved_vmid(struct amdgpu_device *adev,
> +					 struct amdgpu_vm *vm,
> +					 unsigned vmhub)
> +{
> +	struct amdgpu_vm_id_manager *id_mgr;
> +	struct amdgpu_vm_id *idle;
> +	int r = 0;
> +
> +	id_mgr = &adev->vm_manager.id_mgr[vmhub];
> +	mutex_lock(&id_mgr->lock);
> +	if (vm->reserved_vmid[vmhub])
> +		goto unlock;
> +	/* Select the first entry VMID */
> +	idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id, list);
> +	list_del_init(&idle->list);

Could you give a hint how to handle the busy id, what if it's not idle when 
allocating the reserved vmid?

Jerry

> +	vm->reserved_vmid[vmhub] = idle;
> +	mutex_unlock(&id_mgr->lock);
> +
> +	return 0;
> +unlock:
> +	mutex_unlock(&id_mgr->lock);
> +	return r;
> +}
> +
>   static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
>   {
>   	struct amdgpu_device *adev = ring->adev;
> @@ -2316,18 +2355,8 @@ 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->reserved_vmid[i]) {
> -			list_add(&vm->reserved_vmid[i]->list,
> -				 &id_mgr->ids_lru);
> -			vm->reserved_vmid[i] = NULL;
> -		}
> -		mutex_unlock(&id_mgr->lock);
> -	}
> +	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
> +		amdgpu_vm_free_reserved_vmid(adev, vm, i);
>   }
>
>   /**
> @@ -2400,11 +2429,18 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	union drm_amdgpu_vm *args = data;
>   	struct amdgpu_device *adev = dev->dev_private;
>   	struct amdgpu_fpriv *fpriv = filp->driver_priv;
> +	int r;
>
>   	switch (args->in.op) {
>   	case AMDGPU_VM_OP_RESERVE_VMID:
> +		/* current, we only have requirement to reserve vmid from gfxhub */
> +		r = amdgpu_vm_alloc_reserved_vmid(adev, &fpriv->vm,
> +						  AMDGPU_GFXHUB);
> +		if (r)
> +			return r;
> +		break;
>   	case AMDGPU_VM_OP_UNRESERVE_VMID:
> -		return -EINVAL;
> +		amdgpu_vm_free_reserved_vmid(adev, &fpriv->vm, AMDGPU_GFXHUB);
>   		break;
>   	default:
>   		return -EINVAL;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/6] drm/amdgpu: add limitation for dedicated vm number v4

[Chunming Zhou]

v2: move #define to amdgpu_vm.h
v3: move reserved vmid counter to id_manager,
and increase counter before allocating vmid
v4: rename to reserved_vmid_num

Change-Id: Ie5958cf6dbdc1c8278e61d9158483472d6f5c6e3
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index e37421e..e6fdfa4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -557,6 +557,7 @@ static void amdgpu_vm_free_reserved_vmid(struct amdgpu_device *adev,
 		list_add(&vm->reserved_vmid[vmhub]->list,
 			&id_mgr->ids_lru);
 		vm->reserved_vmid[vmhub] = NULL;
+		atomic_dec(&id_mgr->reserved_vmid_num);
 	}
 	mutex_unlock(&id_mgr->lock);
 }
@@ -573,6 +574,13 @@ static int amdgpu_vm_alloc_reserved_vmid(struct amdgpu_device *adev,
 	mutex_lock(&id_mgr->lock);
 	if (vm->reserved_vmid[vmhub])
 		goto unlock;
+	if (atomic_inc_return(&id_mgr->reserved_vmid_num) >
+	    AMDGPU_VM_MAX_RESERVED_VMID) {
+		DRM_ERROR("Over limitation of reserved vmid\n");
+		atomic_dec(&id_mgr->reserved_vmid_num);
+		r = -EINVAL;
+		goto unlock;
+	}
 	/* Select the first entry VMID */
 	idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id, list);
 	list_del_init(&idle->list);
@@ -2376,6 +2384,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
 
 		mutex_init(&id_mgr->lock);
 		INIT_LIST_HEAD(&id_mgr->ids_lru);
+		atomic_set(&id_mgr->reserved_vmid_num, 0);
 
 		/* skip over VMID 0, since it is the system VM */
 		for (j = 1; j < id_mgr->num_ids; ++j) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 8eedca1..9828fcd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -84,6 +84,8 @@
 
 /* hardcode that limit for now */
 #define AMDGPU_VA_RESERVED_SIZE			(8 << 20)
+/* max vmids dedicated for process */
+#define AMDGPU_VM_MAX_RESERVED_VMID	1
 
 struct amdgpu_vm_pt {
 	struct amdgpu_bo	*bo;
@@ -157,6 +159,7 @@ struct amdgpu_vm_id_manager {
 	unsigned		num_ids;
 	struct list_head	ids_lru;
 	struct amdgpu_vm_id	ids[AMDGPU_NUM_VM];
+	atomic_t		reserved_vmid_num;
 };
 
 struct amdgpu_vm_manager {
-- 
1.9.1

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

[PATCH 5/6] drm/amdgpu: implement grab reserved vmid V3

[Chunming Zhou]

v2: move sync waiting only when flush needs
v3: fix racy

Change-Id: I64da2701c9fdcf986afb90ba1492a78d5bef1b6c
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 72 ++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index e6fdfa4..7752279 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -397,6 +397,65 @@ static bool amdgpu_vm_had_gpu_reset(struct amdgpu_device *adev,
 		atomic_read(&adev->gpu_reset_counter);
 }
 
+static bool amdgpu_vm_reserved_vmid_ready(struct amdgpu_vm *vm, unsigned vmhub)
+{
+	return !!vm->reserved_vmid[vmhub];
+}
+
+/* idr_mgr->lock must be held */
+static int amdgpu_vm_grab_reserved_vmid_locked(struct amdgpu_vm *vm,
+					       struct amdgpu_ring *ring,
+					       struct amdgpu_sync *sync,
+					       struct fence *fence,
+					       struct amdgpu_job *job)
+{
+	struct amdgpu_device *adev = ring->adev;
+	unsigned vmhub = ring->funcs->vmhub;
+	struct amdgpu_vm_id *id = vm->reserved_vmid[vmhub];
+	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
+	struct fence *updates = sync->last_vm_update;
+	int r = 0;
+	struct fence *flushed, *tmp;
+	bool needs_flush = false;
+
+	flushed  = id->flushed_updates;
+	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)))) {
+		needs_flush = true;
+		tmp = amdgpu_sync_get_fence(&id->active);
+		if (tmp) {
+			r = amdgpu_sync_fence(adev, sync, tmp);
+			fence_put(tmp);
+			return r;
+		}
+	}
+
+	/* Good we can use this VMID. Remember this submission as
+	* user of the VMID.
+	*/
+	r = amdgpu_sync_fence(ring->adev, &id->active, fence);
+	if (r)
+		goto out;
+
+	if (updates && (!flushed || updates->context != flushed->context ||
+			fence_is_later(updates, flushed))) {
+		fence_put(id->flushed_updates);
+		id->flushed_updates = fence_get(updates);
+	}
+	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;
+
+	job->vm_id = id - id_mgr->ids;
+	trace_amdgpu_vm_grab_id(vm, ring, job);
+out:
+	return r;
+}
+
 /**
  * amdgpu_vm_grab_id - allocate the next free VMID
  *
@@ -421,12 +480,17 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 	unsigned i;
 	int r = 0;
 
+	mutex_lock(&id_mgr->lock);
+	if (amdgpu_vm_reserved_vmid_ready(vm, vmhub)) {
+		r = amdgpu_vm_grab_reserved_vmid_locked(vm, ring, sync, fence, job);
+		mutex_unlock(&id_mgr->lock);
+		return r;
+	}
 	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
-	if (!fences)
+	if (!fences) {
+		mutex_unlock(&id_mgr->lock);
 		return -ENOMEM;
-
-	mutex_lock(&id_mgr->lock);
-
+	}
 	/* Check if we have an idle VMID */
 	i = 0;
 	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
-- 
1.9.1

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

Re: [PATCH 5/6] drm/amdgpu: implement grab reserved vmid V3

[Zhang, Jerry (Junwei)]

On 04/27/2017 01:00 PM, Chunming Zhou wrote:
> v2: move sync waiting only when flush needs
> v3: fix racy
>
> Change-Id: I64da2701c9fdcf986afb90ba1492a78d5bef1b6c
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 72 ++++++++++++++++++++++++++++++++--
>   1 file changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index e6fdfa4..7752279 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -397,6 +397,65 @@ static bool amdgpu_vm_had_gpu_reset(struct amdgpu_device *adev,
>   		atomic_read(&adev->gpu_reset_counter);
>   }
>
> +static bool amdgpu_vm_reserved_vmid_ready(struct amdgpu_vm *vm, unsigned vmhub)
> +{
> +	return !!vm->reserved_vmid[vmhub];
> +}

It may be better to populate it into alloc reserved_vmid func as well, getting 
easy maintenance in the future.


> +
> +/* idr_mgr->lock must be held */
> +static int amdgpu_vm_grab_reserved_vmid_locked(struct amdgpu_vm *vm,
> +					       struct amdgpu_ring *ring,
> +					       struct amdgpu_sync *sync,
> +					       struct fence *fence,
> +					       struct amdgpu_job *job)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	unsigned vmhub = ring->funcs->vmhub;
> +	struct amdgpu_vm_id *id = vm->reserved_vmid[vmhub];
> +	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> +	struct fence *updates = sync->last_vm_update;
> +	int r = 0;
> +	struct fence *flushed, *tmp;
> +	bool needs_flush = false;
> +
> +	flushed  = id->flushed_updates;
> +	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)))) {
> +		needs_flush = true;
> +		tmp = amdgpu_sync_get_fence(&id->active);
> +		if (tmp) {
> +			r = amdgpu_sync_fence(adev, sync, tmp);
> +			fence_put(tmp);
> +			return r;

Sorry, I didn't catch up this.
Could you give me some tips?
(in this case, we no need to update id info as below?)

Jerry

> +		}
> +	}
> +
> +	/* Good we can use this VMID. Remember this submission as
> +	* user of the VMID.
> +	*/
> +	r = amdgpu_sync_fence(ring->adev, &id->active, fence);
> +	if (r)
> +		goto out;
> +
> +	if (updates && (!flushed || updates->context != flushed->context ||
> +			fence_is_later(updates, flushed))) {
> +		fence_put(id->flushed_updates);
> +		id->flushed_updates = fence_get(updates);
> +	}
> +	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;
> +
> +	job->vm_id = id - id_mgr->ids;
> +	trace_amdgpu_vm_grab_id(vm, ring, job);
> +out:
> +	return r;
> +}
> +
>   /**
>    * amdgpu_vm_grab_id - allocate the next free VMID
>    *
> @@ -421,12 +480,17 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   	unsigned i;
>   	int r = 0;
>
> +	mutex_lock(&id_mgr->lock);
> +	if (amdgpu_vm_reserved_vmid_ready(vm, vmhub)) {
> +		r = amdgpu_vm_grab_reserved_vmid_locked(vm, ring, sync, fence, job);
> +		mutex_unlock(&id_mgr->lock);
> +		return r;
> +	}
>   	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
> -	if (!fences)
> +	if (!fences) {
> +		mutex_unlock(&id_mgr->lock);
>   		return -ENOMEM;
> -
> -	mutex_lock(&id_mgr->lock);
> -
> +	}
>   	/* Check if we have an idle VMID */
>   	i = 0;
>   	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/6] drm/amdgpu: implement grab reserved vmid V3

[Zhang, Jerry (Junwei)]

On 04/27/2017 01:54 PM, Zhang, Jerry (Junwei) wrote:
> On 04/27/2017 01:00 PM, Chunming Zhou wrote:
>> v2: move sync waiting only when flush needs
>> v3: fix racy
>>
>> Change-Id: I64da2701c9fdcf986afb90ba1492a78d5bef1b6c
>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 72 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 68 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index e6fdfa4..7752279 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -397,6 +397,65 @@ static bool amdgpu_vm_had_gpu_reset(struct amdgpu_device
>> *adev,
>>           atomic_read(&adev->gpu_reset_counter);
>>   }
>>
>> +static bool amdgpu_vm_reserved_vmid_ready(struct amdgpu_vm *vm, unsigned vmhub)
>> +{
>> +    return !!vm->reserved_vmid[vmhub];
>> +}
>
> It may be better to populate it into alloc reserved_vmid func as well, getting
> easy maintenance in the future.
>
>
>> +
>> +/* idr_mgr->lock must be held */
>> +static int amdgpu_vm_grab_reserved_vmid_locked(struct amdgpu_vm *vm,
>> +                           struct amdgpu_ring *ring,
>> +                           struct amdgpu_sync *sync,
>> +                           struct fence *fence,
>> +                           struct amdgpu_job *job)
>> +{
>> +    struct amdgpu_device *adev = ring->adev;
>> +    unsigned vmhub = ring->funcs->vmhub;
>> +    struct amdgpu_vm_id *id = vm->reserved_vmid[vmhub];
>> +    struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
>> +    struct fence *updates = sync->last_vm_update;
>> +    int r = 0;
>> +    struct fence *flushed, *tmp;
>> +    bool needs_flush = false;
>> +
>> +    flushed  = id->flushed_updates;
>> +    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)))) {
>> +        needs_flush = true;
>> +        tmp = amdgpu_sync_get_fence(&id->active);
>> +        if (tmp) {
>> +            r = amdgpu_sync_fence(adev, sync, tmp);
>> +            fence_put(tmp);
>> +            return r;
>
> Sorry, I didn't catch up this.
> Could you give me some tips?
> (in this case, we no need to update id info as below?)

Got that offline to wait for id idle.
That will always use the reserved id when it's idle.
Anyway, it should work.

>
> Jerry
>
>> +        }
>> +    }
>> +
>> +    /* Good we can use this VMID. Remember this submission as
>> +    * user of the VMID.
>> +    */
>> +    r = amdgpu_sync_fence(ring->adev, &id->active, fence);
>> +    if (r)
>> +        goto out;
>> +
>> +    if (updates && (!flushed || updates->context != flushed->context ||
>> +            fence_is_later(updates, flushed))) {
>> +        fence_put(id->flushed_updates);
>> +        id->flushed_updates = fence_get(updates);
>> +    }
>> +    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;
>> +
>> +    job->vm_id = id - id_mgr->ids;
>> +    trace_amdgpu_vm_grab_id(vm, ring, job);
>> +out:
>> +    return r;
>> +}
>> +
>>   /**
>>    * amdgpu_vm_grab_id - allocate the next free VMID
>>    *
>> @@ -421,12 +480,17 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct
>> amdgpu_ring *ring,
>>       unsigned i;
>>       int r = 0;
>>
>> +    mutex_lock(&id_mgr->lock);
>> +    if (amdgpu_vm_reserved_vmid_ready(vm, vmhub)) {
>> +        r = amdgpu_vm_grab_reserved_vmid_locked(vm, ring, sync, fence, job);
>> +        mutex_unlock(&id_mgr->lock);
>> +        return r;
>> +    }
>>       fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
>> -    if (!fences)
>> +    if (!fences) {
>> +        mutex_unlock(&id_mgr->lock);
>>           return -ENOMEM;
>> -
>> -    mutex_lock(&id_mgr->lock);
>> -
>> +    }
>>       /* Check if we have an idle VMID */
>>       i = 0;
>>       list_for_each_entry(idle, &id_mgr->ids_lru, list) {
>>
> _______________________________________________
> 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

[PATCH 6/6] drm/amdgpu: bump module verion for reserved vmid

[Chunming Zhou]

Change-Id: I1065e0430ed44f7ee6c29214b72e35a7343ea02b
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 55322b4..6799829 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -64,9 +64,10 @@
  * - 3.12.0 - Add query for double offchip LDS buffers
  * - 3.13.0 - Add PRT support
  * - 3.14.0 - Fix race in amdgpu_ctx_get_fence() and note new functionality
+ * - 3.15.0 - Add reserved vmid support
  */
 #define KMS_DRIVER_MAJOR	3
-#define KMS_DRIVER_MINOR	14
+#define KMS_DRIVER_MINOR	15
 #define KMS_DRIVER_PATCHLEVEL	0
 
 int amdgpu_vram_limit = 0;
-- 
1.9.1

_______________________________________________
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.Zhang@amd.com>

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

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 
<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.

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

> +		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.

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(&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;

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 ***

[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@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

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 
>> <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?

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

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

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

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

v5:
  patch#5: peek_fence instead of get_fence.
	   fix potential context starved.

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 V4
  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  | 159 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |   6 ++
 include/uapi/drm/amdgpu_drm.h           |  22 +++++
 5 files changed, 185 insertions(+), 6 deletions(-)

-- 
1.9.1

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

[PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v4

[Chunming Zhou]

add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
v4:
fix racy condition

Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 62 +++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 261aaea..e37421e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -546,6 +546,45 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 	return r;
 }
 
+static void amdgpu_vm_free_reserved_vmid(struct amdgpu_device *adev,
+					  struct amdgpu_vm *vm,
+					  unsigned vmhub)
+{
+	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
+
+	mutex_lock(&id_mgr->lock);
+	if (vm->reserved_vmid[vmhub]) {
+		list_add(&vm->reserved_vmid[vmhub]->list,
+			&id_mgr->ids_lru);
+		vm->reserved_vmid[vmhub] = NULL;
+	}
+	mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_reserved_vmid(struct amdgpu_device *adev,
+					 struct amdgpu_vm *vm,
+					 unsigned vmhub)
+{
+	struct amdgpu_vm_id_manager *id_mgr;
+	struct amdgpu_vm_id *idle;
+	int r = 0;
+
+	id_mgr = &adev->vm_manager.id_mgr[vmhub];
+	mutex_lock(&id_mgr->lock);
+	if (vm->reserved_vmid[vmhub])
+		goto unlock;
+	/* 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);
+
+	return 0;
+unlock:
+	mutex_unlock(&id_mgr->lock);
+	return r;
+}
+
 static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
 {
 	struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2355,8 @@ 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->reserved_vmid[i]) {
-			list_add(&vm->reserved_vmid[i]->list,
-				 &id_mgr->ids_lru);
-			vm->reserved_vmid[i] = NULL;
-		}
-		mutex_unlock(&id_mgr->lock);
-	}
+	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+		amdgpu_vm_free_reserved_vmid(adev, vm, i);
 }
 
 /**
@@ -2400,11 +2429,18 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 	union drm_amdgpu_vm *args = data;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_fpriv *fpriv = filp->driver_priv;
+	int r;
 
 	switch (args->in.op) {
 	case AMDGPU_VM_OP_RESERVE_VMID:
+		/* current, we only have requirement to reserve vmid from gfxhub */
+		r = amdgpu_vm_alloc_reserved_vmid(adev, &fpriv->vm,
+						  AMDGPU_GFXHUB);
+		if (r)
+			return r;
+		break;
 	case AMDGPU_VM_OP_UNRESERVE_VMID:
-		return -EINVAL;
+		amdgpu_vm_free_reserved_vmid(adev, &fpriv->vm, AMDGPU_GFXHUB);
 		break;
 	default:
 		return -EINVAL;
-- 
1.9.1

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