Re: [PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3
Am 27.04.2017 um 04:24 schrieb Zhang, Jerry (Junwei):
On 04/27/2017 10:13 AM, zhoucm1 wrote:
On 2017年04月26日 20:26, Christian König wrote:
Am 26.04.2017 um 13:10 schrieb Chunming Zhou:
add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 70
+++---
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ec87d64..a783e8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -397,6 +397,11 @@ static bool amdgpu_vm_had_gpu_reset(struct
amdgpu_device *adev,
atomic_read(&adev->gpu_reset_counter);
}
+static bool amdgpu_vm_dedicated_vmid_ready(struct amdgpu_vm *vm,
unsigned
vmhub)
+{
+return !!vm->dedicated_vmid[vmhub];
+}
+
/**
* amdgpu_vm_grab_id - allocate the next free VMID
*
@@ -546,6 +551,47 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm,
struct
amdgpu_ring *ring,
return r;
}
+static void amdgpu_vm_free_dedicated_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->dedicated_vmid[vmhub]) {
+ list_add(&vm->dedicated_vmid[vmhub]->list,
+&id_mgr->ids_lru);
+vm->dedicated_vmid[vmhub] = NULL;
+}
+mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_dedicated_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;
+
+id_mgr = &adev->vm_manager.id_mgr[vmhub];
+mutex_lock(&id_mgr->lock);
+/* Select the first entry VMID */
+idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id,
list);
+list_del_init(&idle->list);
+vm->dedicated_vmid[vmhub] = idle;
We need a check here if there isn't an ID already reserved for the VM.
Additional to that I would still rename the field to reserved_vmid,
that
describes better what it is actually doing.
the vmid is global, reserve it from global lru, so it makes sense to
name
'reserved_vmid' like in patch#4.
the reserved vmid is dedicated to one vm, so the field in vm is
'dedicated_vmid' like here, which is my reason to name it.
Just the same thing looking at different views.
IMHO, it's reserved vmid from global group, naming "reserved_vmid".
And in Patch#4, it constrains the number, possibly naming
"reserved_vmid_seq" or "reserved_vmid_ref".
Any of them looks reasonable, select the one you like. :)
Yeah, agree. More like a gut feeling that reserved is more common
wording, but I don't have a strong opinion on that either.
Christian.
Jerry
Anyway, the alternative is ok to me although I prefer mine, I hope we
can
deliver this solution by today, many RGP issues depend on it.
Thanks,
David Zhou
+ mutex_unlock(&id_mgr->lock);
+
+r = amdgpu_sync_wait(&idle->active);
+if (r)
+goto err;
This is racy. A command submission could happen while we wait for
the id to
become idle.
The easiest way to solve this is to hold the lock while waiting for
the ID to
become available.
+
+return 0;
+err:
+amdgpu_vm_free_dedicated_vmid(adev, vm, vmhub);
+return r;
+}
+
static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring
*ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2362,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->dedicated_vmid[i]) {
- list_add(&vm->dedicated_vmid[i]->list,
- &id_mgr->ids_lru);
-vm->dedicated_vmid[i] = NULL;
-}
-mutex_unlock(&id_mgr->lock);
-}
+for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+amdgpu_vm_free_dedicated_vmid(adev, vm, i);
}
/**
@@ -2400,11 +2436,19 @@ 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 */
+if (!amdgpu_vm_dedicated_vmid_ready(&fpriv->vm, 0)) {
+r = amdgpu_vm_alloc_dedicated_vmid(adev, &fpriv->vm, 0);
This is racy as well.
Two threads could try to reserve a VMID at the same time. You need to
integ
Re: [PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3
On 2017年04月27日 10:24, Zhang, Jerry (Junwei) wrote:
On 04/27/2017 10:13 AM, zhoucm1 wrote:
On 2017年04月26日 20:26, Christian König wrote:
Am 26.04.2017 um 13:10 schrieb Chunming Zhou:
add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 70
+++---
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ec87d64..a783e8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -397,6 +397,11 @@ static bool amdgpu_vm_had_gpu_reset(struct
amdgpu_device *adev,
atomic_read(&adev->gpu_reset_counter);
}
+static bool amdgpu_vm_dedicated_vmid_ready(struct amdgpu_vm *vm,
unsigned
vmhub)
+{
+return !!vm->dedicated_vmid[vmhub];
+}
+
/**
* amdgpu_vm_grab_id - allocate the next free VMID
*
@@ -546,6 +551,47 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm,
struct
amdgpu_ring *ring,
return r;
}
+static void amdgpu_vm_free_dedicated_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->dedicated_vmid[vmhub]) {
+ list_add(&vm->dedicated_vmid[vmhub]->list,
+&id_mgr->ids_lru);
+vm->dedicated_vmid[vmhub] = NULL;
+}
+mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_dedicated_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;
+
+id_mgr = &adev->vm_manager.id_mgr[vmhub];
+mutex_lock(&id_mgr->lock);
+/* Select the first entry VMID */
+idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id,
list);
+list_del_init(&idle->list);
+vm->dedicated_vmid[vmhub] = idle;
We need a check here if there isn't an ID already reserved for the VM.
Additional to that I would still rename the field to reserved_vmid,
that
describes better what it is actually doing.
the vmid is global, reserve it from global lru, so it makes sense to
name
'reserved_vmid' like in patch#4.
the reserved vmid is dedicated to one vm, so the field in vm is
'dedicated_vmid' like here, which is my reason to name it.
Just the same thing looking at different views.
IMHO, it's reserved vmid from global group, naming "reserved_vmid".
And in Patch#4, it constrains the number, possibly naming
"reserved_vmid_seq" or "reserved_vmid_ref".
How about reserved_vmid_num?
Regards,
David Zhou
Any of them looks reasonable, select the one you like. :)
Jerry
Anyway, the alternative is ok to me although I prefer mine, I hope we
can
deliver this solution by today, many RGP issues depend on it.
Thanks,
David Zhou
+ mutex_unlock(&id_mgr->lock);
+
+r = amdgpu_sync_wait(&idle->active);
+if (r)
+goto err;
This is racy. A command submission could happen while we wait for
the id to
become idle.
The easiest way to solve this is to hold the lock while waiting for
the ID to
become available.
+
+return 0;
+err:
+amdgpu_vm_free_dedicated_vmid(adev, vm, vmhub);
+return r;
+}
+
static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring
*ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2362,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->dedicated_vmid[i]) {
- list_add(&vm->dedicated_vmid[i]->list,
- &id_mgr->ids_lru);
-vm->dedicated_vmid[i] = NULL;
-}
-mutex_unlock(&id_mgr->lock);
-}
+for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+amdgpu_vm_free_dedicated_vmid(adev, vm, i);
}
/**
@@ -2400,11 +2436,19 @@ 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 */
+if (!amdgpu_vm_dedicated_vmid_ready(&fpriv->vm, 0)) {
+r = amdgpu_vm_alloc_dedicated_vmid(adev, &fpriv->vm, 0);
This is racy as well.
Two threads could try to reserve a VMID at the same time. You need to
integrate the check if it's already allocated into
amdgpu_vm_alloc_dedicated_vmid().
Regards,
Chr
Re: [PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3
On 04/27/2017 10:13 AM, zhoucm1 wrote:
On 2017年04月26日 20:26, Christian König wrote:
Am 26.04.2017 um 13:10 schrieb Chunming Zhou:
add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 70
+++---
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ec87d64..a783e8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -397,6 +397,11 @@ static bool amdgpu_vm_had_gpu_reset(struct
amdgpu_device *adev,
atomic_read(&adev->gpu_reset_counter);
}
+static bool amdgpu_vm_dedicated_vmid_ready(struct amdgpu_vm *vm, unsigned
vmhub)
+{
+return !!vm->dedicated_vmid[vmhub];
+}
+
/**
* amdgpu_vm_grab_id - allocate the next free VMID
*
@@ -546,6 +551,47 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct
amdgpu_ring *ring,
return r;
}
+static void amdgpu_vm_free_dedicated_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->dedicated_vmid[vmhub]) {
+list_add(&vm->dedicated_vmid[vmhub]->list,
+&id_mgr->ids_lru);
+vm->dedicated_vmid[vmhub] = NULL;
+}
+mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_dedicated_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;
+
+id_mgr = &adev->vm_manager.id_mgr[vmhub];
+mutex_lock(&id_mgr->lock);
+/* Select the first entry VMID */
+idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id, list);
+list_del_init(&idle->list);
+vm->dedicated_vmid[vmhub] = idle;
We need a check here if there isn't an ID already reserved for the VM.
Additional to that I would still rename the field to reserved_vmid, that
describes better what it is actually doing.
the vmid is global, reserve it from global lru, so it makes sense to name
'reserved_vmid' like in patch#4.
the reserved vmid is dedicated to one vm, so the field in vm is
'dedicated_vmid' like here, which is my reason to name it.
Just the same thing looking at different views.
IMHO, it's reserved vmid from global group, naming "reserved_vmid".
And in Patch#4, it constrains the number, possibly naming "reserved_vmid_seq"
or "reserved_vmid_ref".
Any of them looks reasonable, select the one you like. :)
Jerry
Anyway, the alternative is ok to me although I prefer mine, I hope we can
deliver this solution by today, many RGP issues depend on it.
Thanks,
David Zhou
+mutex_unlock(&id_mgr->lock);
+
+r = amdgpu_sync_wait(&idle->active);
+if (r)
+goto err;
This is racy. A command submission could happen while we wait for the id to
become idle.
The easiest way to solve this is to hold the lock while waiting for the ID to
become available.
+
+return 0;
+err:
+amdgpu_vm_free_dedicated_vmid(adev, vm, vmhub);
+return r;
+}
+
static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2362,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->dedicated_vmid[i]) {
-list_add(&vm->dedicated_vmid[i]->list,
- &id_mgr->ids_lru);
-vm->dedicated_vmid[i] = NULL;
-}
-mutex_unlock(&id_mgr->lock);
-}
+for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+amdgpu_vm_free_dedicated_vmid(adev, vm, i);
}
/**
@@ -2400,11 +2436,19 @@ 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 */
+if (!amdgpu_vm_dedicated_vmid_ready(&fpriv->vm, 0)) {
+r = amdgpu_vm_alloc_dedicated_vmid(adev, &fpriv->vm, 0);
This is racy as well.
Two threads could try to reserve a VMID at the same time. You need to
integrate the check if it's already allocated into
amdgpu_vm_alloc_dedicated_vmid().
Regards,
Christian.
+if (r)
+return r;
+}
+break;
case AMDGPU_VM_OP_U
Re: [PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3
On 04/26/2017 08:26 PM, Christian König wrote:
Am 26.04.2017 um 13:10 schrieb Chunming Zhou:
add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 70 +++---
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ec87d64..a783e8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -397,6 +397,11 @@ static bool amdgpu_vm_had_gpu_reset(struct amdgpu_device
*adev,
atomic_read(&adev->gpu_reset_counter);
}
+static bool amdgpu_vm_dedicated_vmid_ready(struct amdgpu_vm *vm, unsigned
vmhub)
+{
+return !!vm->dedicated_vmid[vmhub];
+}
+
/**
* amdgpu_vm_grab_id - allocate the next free VMID
*
@@ -546,6 +551,47 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct
amdgpu_ring *ring,
return r;
}
+static void amdgpu_vm_free_dedicated_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->dedicated_vmid[vmhub]) {
+list_add(&vm->dedicated_vmid[vmhub]->list,
+&id_mgr->ids_lru);
+vm->dedicated_vmid[vmhub] = NULL;
+}
+mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_dedicated_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;
+
+id_mgr = &adev->vm_manager.id_mgr[vmhub];
+mutex_lock(&id_mgr->lock);
+/* Select the first entry VMID */
+idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id, list);
+list_del_init(&idle->list);
+vm->dedicated_vmid[vmhub] = idle;
We need a check here if there isn't an ID already reserved for the VM.
Additional to that I would still rename the field to reserved_vmid, that
describes better what it is actually doing.
+mutex_unlock(&id_mgr->lock);
+
+r = amdgpu_sync_wait(&idle->active);
+if (r)
+goto err;
This is racy. A command submission could happen while we wait for the id to
become idle.
The easiest way to solve this is to hold the lock while waiting for the ID to
become available.
+
+return 0;
+err:
+amdgpu_vm_free_dedicated_vmid(adev, vm, vmhub);
+return r;
+}
+
static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2362,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->dedicated_vmid[i]) {
-list_add(&vm->dedicated_vmid[i]->list,
- &id_mgr->ids_lru);
-vm->dedicated_vmid[i] = NULL;
-}
-mutex_unlock(&id_mgr->lock);
-}
+for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+amdgpu_vm_free_dedicated_vmid(adev, vm, i);
}
/**
@@ -2400,11 +2436,19 @@ 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 */
+if (!amdgpu_vm_dedicated_vmid_ready(&fpriv->vm, 0)) {
+r = amdgpu_vm_alloc_dedicated_vmid(adev, &fpriv->vm, 0);
This is racy as well.
Two threads could try to reserve a VMID at the same time. You need to integrate
the check if it's already allocated into amdgpu_vm_alloc_dedicated_vmid().
Apart from that, small tip.
AMDGPU_GFXHUB instead of "0" looks more self-explanatory.
Jerry
Regards,
Christian.
+if (r)
+return r;
+}
+break;
case AMDGPU_VM_OP_UNRESERVE_VMID:
-return -EINVAL;
+amdgpu_vm_free_dedicated_vmid(adev, &fpriv->vm, 0);
break;
default:
return -EINVAL;
___
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
Re: [PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3
On 2017年04月26日 20:26, Christian König wrote:
Am 26.04.2017 um 13:10 schrieb Chunming Zhou:
add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 70
+++---
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ec87d64..a783e8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -397,6 +397,11 @@ static bool amdgpu_vm_had_gpu_reset(struct
amdgpu_device *adev,
atomic_read(&adev->gpu_reset_counter);
}
+static bool amdgpu_vm_dedicated_vmid_ready(struct amdgpu_vm *vm,
unsigned vmhub)
+{
+return !!vm->dedicated_vmid[vmhub];
+}
+
/**
* amdgpu_vm_grab_id - allocate the next free VMID
*
@@ -546,6 +551,47 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm,
struct amdgpu_ring *ring,
return r;
}
+static void amdgpu_vm_free_dedicated_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->dedicated_vmid[vmhub]) {
+list_add(&vm->dedicated_vmid[vmhub]->list,
+&id_mgr->ids_lru);
+vm->dedicated_vmid[vmhub] = NULL;
+}
+mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_dedicated_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;
+
+id_mgr = &adev->vm_manager.id_mgr[vmhub];
+mutex_lock(&id_mgr->lock);
+/* Select the first entry VMID */
+idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id,
list);
+list_del_init(&idle->list);
+vm->dedicated_vmid[vmhub] = idle;
We need a check here if there isn't an ID already reserved for the VM.
Additional to that I would still rename the field to reserved_vmid,
that describes better what it is actually doing.
the vmid is global, reserve it from global lru, so it makes sense to
name 'reserved_vmid' like in patch#4.
the reserved vmid is dedicated to one vm, so the field in vm is
'dedicated_vmid' like here, which is my reason to name it.
Anyway, the alternative is ok to me although I prefer mine, I hope we
can deliver this solution by today, many RGP issues depend on it.
Thanks,
David Zhou
+mutex_unlock(&id_mgr->lock);
+
+r = amdgpu_sync_wait(&idle->active);
+if (r)
+goto err;
This is racy. A command submission could happen while we wait for the
id to become idle.
The easiest way to solve this is to hold the lock while waiting for
the ID to become available.
+
+return 0;
+err:
+amdgpu_vm_free_dedicated_vmid(adev, vm, vmhub);
+return r;
+}
+
static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring
*ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2362,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->dedicated_vmid[i]) {
-list_add(&vm->dedicated_vmid[i]->list,
- &id_mgr->ids_lru);
-vm->dedicated_vmid[i] = NULL;
-}
-mutex_unlock(&id_mgr->lock);
-}
+for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+amdgpu_vm_free_dedicated_vmid(adev, vm, i);
}
/**
@@ -2400,11 +2436,19 @@ 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 */
+if (!amdgpu_vm_dedicated_vmid_ready(&fpriv->vm, 0)) {
+r = amdgpu_vm_alloc_dedicated_vmid(adev, &fpriv->vm, 0);
This is racy as well.
Two threads could try to reserve a VMID at the same time. You need to
integrate the check if it's already allocated into
amdgpu_vm_alloc_dedicated_vmid().
Regards,
Christian.
+if (r)
+return r;
+}
+break;
case AMDGPU_VM_OP_UNRESERVE_VMID:
-return -EINVAL;
+amdgpu_vm_free_dedicated_vmid(adev, &fpriv->vm, 0);
break;
default:
return -EINVAL;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mail
Re: [PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3
Am 26.04.2017 um 13:10 schrieb Chunming Zhou:
add reserve/unreserve vmid funtions.
v3:
only reserve vmid from gfxhub
Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
Signed-off-by: Chunming Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 70 +++---
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ec87d64..a783e8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -397,6 +397,11 @@ static bool amdgpu_vm_had_gpu_reset(struct amdgpu_device
*adev,
atomic_read(&adev->gpu_reset_counter);
}
+static bool amdgpu_vm_dedicated_vmid_ready(struct amdgpu_vm *vm, unsigned vmhub)
+{
+ return !!vm->dedicated_vmid[vmhub];
+}
+
/**
* amdgpu_vm_grab_id - allocate the next free VMID
*
@@ -546,6 +551,47 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct
amdgpu_ring *ring,
return r;
}
+static void amdgpu_vm_free_dedicated_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->dedicated_vmid[vmhub]) {
+ list_add(&vm->dedicated_vmid[vmhub]->list,
+ &id_mgr->ids_lru);
+ vm->dedicated_vmid[vmhub] = NULL;
+ }
+ mutex_unlock(&id_mgr->lock);
+}
+
+static int amdgpu_vm_alloc_dedicated_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;
+
+ id_mgr = &adev->vm_manager.id_mgr[vmhub];
+ mutex_lock(&id_mgr->lock);
+ /* Select the first entry VMID */
+ idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id, list);
+ list_del_init(&idle->list);
+ vm->dedicated_vmid[vmhub] = idle;
We need a check here if there isn't an ID already reserved for the VM.
Additional to that I would still rename the field to reserved_vmid, that
describes better what it is actually doing.
+ mutex_unlock(&id_mgr->lock);
+
+ r = amdgpu_sync_wait(&idle->active);
+ if (r)
+ goto err;
This is racy. A command submission could happen while we wait for the id
to become idle.
The easiest way to solve this is to hold the lock while waiting for the
ID to become available.
+
+ return 0;
+err:
+ amdgpu_vm_free_dedicated_vmid(adev, vm, vmhub);
+ return r;
+}
+
static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -2316,18 +2362,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->dedicated_vmid[i]) {
- list_add(&vm->dedicated_vmid[i]->list,
-&id_mgr->ids_lru);
- vm->dedicated_vmid[i] = NULL;
- }
- mutex_unlock(&id_mgr->lock);
- }
+ for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
+ amdgpu_vm_free_dedicated_vmid(adev, vm, i);
}
/**
@@ -2400,11 +2436,19 @@ 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 */
+ if (!amdgpu_vm_dedicated_vmid_ready(&fpriv->vm, 0)) {
+ r = amdgpu_vm_alloc_dedicated_vmid(adev, &fpriv->vm, 0);
This is racy as well.
Two threads could try to reserve a VMID at the same time. You need to
integrate the check if it's already allocated into
amdgpu_vm_alloc_dedicated_vmid().
Regards,
Christian.
+ if (r)
+ return r;
+ }
+ break;
case AMDGPU_VM_OP_UNRESERVE_VMID:
- return -EINVAL;
+ amdgpu_vm_free_dedicated_vmid(adev, &fpriv->vm, 0);
break;
default:
return -EINVAL;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
