Re: [PATCH 1/3] drm/amdgpu: fix IB test MCBP bug
Am 02.03.20 um 10:39 schrieb Liu, Monk:
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
Kernel copies also don't use a VMID, so I think that this won't work correctly.
Do you have the job available as well? That would probably be better to test
for since only IB tests don't use a job.
[ML]
The @job is not passed to the gfx_v10_0_ring_emit_ib_gfx() routine so I tend to
modify as less as possible
Besides, for kernel copies: I believe they also do not support MCBP (can you
point me the code in source of those kernel copies ?) , so use VMID to test is
fine (To support MCBP on gfx ring you need a set of additional preamble IB co
work with your workload iB and kernel copies apparently don't have those stuffs)
Ah, sorry! My incorrect thinking, kernel copies also don't use the CP
but rather the SDMA. So the change is completely irrelevant for the copies.
Acked-by: Christian König for the patch.
Regards,
Christian.
Thanks
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: Christian König
Sent: Monday, March 2, 2020 5:35 PM
To: Liu, Monk ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/amdgpu: fix IB test MCBP bug
Am 02.03.20 um 10:22 schrieb Monk Liu:
1)for gfx IB test we shouldn't insert DE meta data
2)we should make sure IB test finished before we send event 3 to
hypervisor otherwise the IDLE from event 3 will preempt IB test, which
is not designed as a compatible structure for MCBP
Signed-off-by: Monk Liu
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c| 3 ---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 +-
5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 351096a..572eb6e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3195,6 +3195,12 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
flush_delayed_work(>delayed_init_work);
adev->shutdown = true;
+ /* make sure IB test finished before entering exclusive mode
+* to avoid preemption on IB test
+* */
+ if (amdgpu_sriov_vf(adev))
+ amdgpu_virt_request_full_gpu(adev, false);
+
/* disable all interrupts */
amdgpu_irq_disable_all(adev);
if (adev->mode_info.mode_config_initialized){
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 0f35639..0b1511a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -88,9 +88,6 @@ void amdgpu_driver_unload_kms(struct drm_device *dev)
if (adev->rmmio == NULL)
goto done_free;
- if (amdgpu_sriov_vf(adev))
- amdgpu_virt_request_full_gpu(adev, false);
-
if (adev->runpm) {
pm_runtime_get_sync(dev->dev);
pm_runtime_forbid(dev->dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 94ca9ff..0555989 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4432,7 +4432,7 @@ static void gfx_v10_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (flags & AMDGPU_IB_PREEMPTED)
control |= INDIRECT_BUFFER_PRE_RESUME(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
gfx_v10_0_ring_emit_de_meta(ring,
(!amdgpu_sriov_vf(ring->adev) && flags &
AMDGPU_IB_PREEMPTED) ? true : false);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 393a132..b14f46a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6116,7 +6116,7 @@ static void gfx_v8_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (amdgpu_sriov_vf(ring->adev) && (ib->flags &
AMDGPU_IB_FLAG_PREEMPT)) {
control |= INDIRECT_BUFFER_PRE_ENB(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
Kernel copies also don't use a VMID, so I think that this won't work correctly.
Do you have the job available as well? That would probably be better to test
for since only IB tests don't use a job.
Christian.
gfx_v8_0_ring_emit_de_meta(ring);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 0156479..d8d256e6
RE: [PATCH 1/3] drm/amdgpu: fix IB test MCBP bug
> - if (!(ib->flags & AMDGPU_IB_FLAG_CE))
> + if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
Kernel copies also don't use a VMID, so I think that this won't work correctly.
Do you have the job available as well? That would probably be better to test
for since only IB tests don't use a job.
[ML]
The @job is not passed to the gfx_v10_0_ring_emit_ib_gfx() routine so I tend to
modify as less as possible
Besides, for kernel copies: I believe they also do not support MCBP (can you
point me the code in source of those kernel copies ?) , so use VMID to test is
fine (To support MCBP on gfx ring you need a set of additional preamble IB co
work with your workload iB and kernel copies apparently don't have those stuffs)
Thanks
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: Christian König
Sent: Monday, March 2, 2020 5:35 PM
To: Liu, Monk ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/amdgpu: fix IB test MCBP bug
Am 02.03.20 um 10:22 schrieb Monk Liu:
> 1)for gfx IB test we shouldn't insert DE meta data
>
> 2)we should make sure IB test finished before we send event 3 to
> hypervisor otherwise the IDLE from event 3 will preempt IB test, which
> is not designed as a compatible structure for MCBP
>
> Signed-off-by: Monk Liu
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c| 3 ---
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 +-
> 5 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 351096a..572eb6e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3195,6 +3195,12 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
> flush_delayed_work(>delayed_init_work);
> adev->shutdown = true;
>
> + /* make sure IB test finished before entering exclusive mode
> + * to avoid preemption on IB test
> + * */
> + if (amdgpu_sriov_vf(adev))
> + amdgpu_virt_request_full_gpu(adev, false);
> +
> /* disable all interrupts */
> amdgpu_irq_disable_all(adev);
> if (adev->mode_info.mode_config_initialized){
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 0f35639..0b1511a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -88,9 +88,6 @@ void amdgpu_driver_unload_kms(struct drm_device *dev)
> if (adev->rmmio == NULL)
> goto done_free;
>
> - if (amdgpu_sriov_vf(adev))
> - amdgpu_virt_request_full_gpu(adev, false);
> -
> if (adev->runpm) {
> pm_runtime_get_sync(dev->dev);
> pm_runtime_forbid(dev->dev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 94ca9ff..0555989 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -4432,7 +4432,7 @@ static void gfx_v10_0_ring_emit_ib_gfx(struct
> amdgpu_ring *ring,
> if (flags & AMDGPU_IB_PREEMPTED)
> control |= INDIRECT_BUFFER_PRE_RESUME(1);
>
> - if (!(ib->flags & AMDGPU_IB_FLAG_CE))
> + if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
> gfx_v10_0_ring_emit_de_meta(ring,
> (!amdgpu_sriov_vf(ring->adev) && flags &
> AMDGPU_IB_PREEMPTED) ? true : false);
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 393a132..b14f46a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -6116,7 +6116,7 @@ static void gfx_v8_0_ring_emit_ib_gfx(struct
> amdgpu_ring *ring,
> if (amdgpu_sriov_vf(ring->adev) && (ib->flags &
> AMDGPU_IB_FLAG_PREEMPT)) {
> control |= INDIRECT_BUFFER_PRE_ENB(1);
>
> - if (!(ib->flags & AMDGPU_IB_FLAG_CE))
> + if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
Kernel copies also don't use a VMID, so I think that this won't work correctly.
Do you have the job available as well? That would probably be better to test
for since only IB tests don't use a job.
Christian.
> gfx_v8_0_ring_emit_de_meta(ring);
> }
>
Re: [PATCH 1/3] drm/amdgpu: fix IB test MCBP bug
Am 02.03.20 um 10:22 schrieb Monk Liu:
1)for gfx IB test we shouldn't insert DE meta data
2)we should make sure IB test finished before we
send event 3 to hypervisor otherwise the IDLE from
event 3 will preempt IB test, which is not designed
as a compatible structure for MCBP
Signed-off-by: Monk Liu
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c| 3 ---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 +-
5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 351096a..572eb6e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3195,6 +3195,12 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
flush_delayed_work(>delayed_init_work);
adev->shutdown = true;
+ /* make sure IB test finished before entering exclusive mode
+* to avoid preemption on IB test
+* */
+ if (amdgpu_sriov_vf(adev))
+ amdgpu_virt_request_full_gpu(adev, false);
+
/* disable all interrupts */
amdgpu_irq_disable_all(adev);
if (adev->mode_info.mode_config_initialized){
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 0f35639..0b1511a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -88,9 +88,6 @@ void amdgpu_driver_unload_kms(struct drm_device *dev)
if (adev->rmmio == NULL)
goto done_free;
- if (amdgpu_sriov_vf(adev))
- amdgpu_virt_request_full_gpu(adev, false);
-
if (adev->runpm) {
pm_runtime_get_sync(dev->dev);
pm_runtime_forbid(dev->dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 94ca9ff..0555989 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4432,7 +4432,7 @@ static void gfx_v10_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (flags & AMDGPU_IB_PREEMPTED)
control |= INDIRECT_BUFFER_PRE_RESUME(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
gfx_v10_0_ring_emit_de_meta(ring,
(!amdgpu_sriov_vf(ring->adev) && flags &
AMDGPU_IB_PREEMPTED) ? true : false);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 393a132..b14f46a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6116,7 +6116,7 @@ static void gfx_v8_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (amdgpu_sriov_vf(ring->adev) && (ib->flags &
AMDGPU_IB_FLAG_PREEMPT)) {
control |= INDIRECT_BUFFER_PRE_ENB(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
Kernel copies also don't use a VMID, so I think that this won't work
correctly.
Do you have the job available as well? That would probably be better to
test for since only IB tests don't use a job.
Christian.
gfx_v8_0_ring_emit_de_meta(ring);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 0156479..d8d256e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -4985,7 +4985,7 @@ static void gfx_v9_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (amdgpu_sriov_vf(ring->adev) && (ib->flags &
AMDGPU_IB_FLAG_PREEMPT)) {
control |= INDIRECT_BUFFER_PRE_ENB(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
gfx_v9_0_ring_emit_de_meta(ring);
}
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/3] drm/amdgpu: fix IB test MCBP bug
1)for gfx IB test we shouldn't insert DE meta data
2)we should make sure IB test finished before we
send event 3 to hypervisor otherwise the IDLE from
event 3 will preempt IB test, which is not designed
as a compatible structure for MCBP
Signed-off-by: Monk Liu
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c| 3 ---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 +-
5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 351096a..572eb6e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3195,6 +3195,12 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
flush_delayed_work(>delayed_init_work);
adev->shutdown = true;
+ /* make sure IB test finished before entering exclusive mode
+* to avoid preemption on IB test
+* */
+ if (amdgpu_sriov_vf(adev))
+ amdgpu_virt_request_full_gpu(adev, false);
+
/* disable all interrupts */
amdgpu_irq_disable_all(adev);
if (adev->mode_info.mode_config_initialized){
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 0f35639..0b1511a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -88,9 +88,6 @@ void amdgpu_driver_unload_kms(struct drm_device *dev)
if (adev->rmmio == NULL)
goto done_free;
- if (amdgpu_sriov_vf(adev))
- amdgpu_virt_request_full_gpu(adev, false);
-
if (adev->runpm) {
pm_runtime_get_sync(dev->dev);
pm_runtime_forbid(dev->dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 94ca9ff..0555989 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4432,7 +4432,7 @@ static void gfx_v10_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (flags & AMDGPU_IB_PREEMPTED)
control |= INDIRECT_BUFFER_PRE_RESUME(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
gfx_v10_0_ring_emit_de_meta(ring,
(!amdgpu_sriov_vf(ring->adev) && flags &
AMDGPU_IB_PREEMPTED) ? true : false);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 393a132..b14f46a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6116,7 +6116,7 @@ static void gfx_v8_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (amdgpu_sriov_vf(ring->adev) && (ib->flags &
AMDGPU_IB_FLAG_PREEMPT)) {
control |= INDIRECT_BUFFER_PRE_ENB(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
gfx_v8_0_ring_emit_de_meta(ring);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 0156479..d8d256e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -4985,7 +4985,7 @@ static void gfx_v9_0_ring_emit_ib_gfx(struct amdgpu_ring
*ring,
if (amdgpu_sriov_vf(ring->adev) && (ib->flags &
AMDGPU_IB_FLAG_PREEMPT)) {
control |= INDIRECT_BUFFER_PRE_ENB(1);
- if (!(ib->flags & AMDGPU_IB_FLAG_CE))
+ if (!(ib->flags & AMDGPU_IB_FLAG_CE) && vmid)
gfx_v9_0_ring_emit_de_meta(ring);
}
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
