* [RFC PATCH 2/2] drm/amdgpu: protect ring from concurrency access
2021-09-13 5:55 [RFC PATCH 1/2] drm/amdgpu: Introduce ring lock xinhui pan
@ 2021-09-13 5:55 ` xinhui pan
2021-09-13 6:25 ` [RFC PATCH 1/2] drm/amdgpu: Introduce ring lock Christian König
1 sibling, 0 replies; 3+ messages in thread
From: xinhui pan @ 2021-09-13 5:55 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, christian.koenig, lijo.lazar, xinhui pan
Park the ring scheduler thread before accessing the ring.
And unpark it only when we finish accessing the ring.
The right sequence should be like below.
lock ring
park ring thread
direct access ring
[unlock ring, do something, lock ring]
unpark ring thread
unlock ring
Signed-off-by: xinhui pan <xinhui.pan@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 30 ++++++---------------
drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 6 +++++
2 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 19323b4cce7b..5138d5b52c9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1349,7 +1349,7 @@ static int amdgpu_debugfs_test_ib_show(struct seq_file *m, void *unused)
{
struct amdgpu_device *adev = (struct amdgpu_device *)m->private;
struct drm_device *dev = adev_to_drm(adev);
- int r = 0, i;
+ int r = 0;
r = pm_runtime_get_sync(dev->dev);
if (r < 0) {
@@ -1362,15 +1362,6 @@ static int amdgpu_debugfs_test_ib_show(struct seq_file *m, void *unused)
if (r)
return r;
- /* hold on the scheduler */
- for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
- struct amdgpu_ring *ring = adev->rings[i];
-
- if (!ring || !ring->sched.thread)
- continue;
- kthread_park(ring->sched.thread);
- }
-
seq_printf(m, "run ib test:\n");
r = amdgpu_ib_ring_tests(adev);
if (r)
@@ -1378,15 +1369,6 @@ static int amdgpu_debugfs_test_ib_show(struct seq_file *m, void *unused)
else
seq_printf(m, "ib ring tests passed.\n");
- /* go on the scheduler */
- for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
- struct amdgpu_ring *ring = adev->rings[i];
-
- if (!ring || !ring->sched.thread)
- continue;
- kthread_unpark(ring->sched.thread);
- }
-
up_read(&adev->reset_sem);
pm_runtime_mark_last_busy(dev->dev);
@@ -1650,13 +1632,14 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val)
if (r)
goto pro_end;
- /* stop the scheduler */
- kthread_park(ring->sched.thread);
-
resched = ttm_bo_lock_delayed_workqueue(&adev->mman.bdev);
/* preempt the IB */
+ mutex_lock(&ring->lock);
+ /* stop the scheduler */
+ kthread_park(ring->sched.thread);
r = amdgpu_ring_preempt_ib(ring);
+ mutex_unlock(&ring->lock);
if (r) {
DRM_WARN("failed to preempt ring %d\n", ring->idx);
goto failure;
@@ -1686,8 +1669,11 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val)
}
failure:
+ /* make sure thread is not unparked accidently. */
+ mutex_lock(&ring->lock);
/* restart the scheduler */
kthread_unpark(ring->sched.thread);
+ mutex_unlock(&ring->lock);
up_read(&adev->reset_sem);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 9274f32c3661..ea83b3aef8fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -402,7 +402,13 @@ int amdgpu_ib_ring_tests(struct amdgpu_device *adev)
else
tmo = tmo_gfx;
+ mutex_lock(&ring->lock);
+ if (ring->sched.thread)
+ kthread_park(ring->sched.thread);
r = amdgpu_ring_test_ib(ring, tmo);
+ if (ring->sched.thread)
+ kthread_unpark(ring->sched.thread);
+ mutex_unlock(&ring->lock);
if (!r) {
DRM_DEV_DEBUG(adev->dev, "ib test on %s succeeded\n",
ring->name);
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [RFC PATCH 1/2] drm/amdgpu: Introduce ring lock
2021-09-13 5:55 [RFC PATCH 1/2] drm/amdgpu: Introduce ring lock xinhui pan
2021-09-13 5:55 ` [RFC PATCH 2/2] drm/amdgpu: protect ring from concurrency access xinhui pan
@ 2021-09-13 6:25 ` Christian König
1 sibling, 0 replies; 3+ messages in thread
From: Christian König @ 2021-09-13 6:25 UTC (permalink / raw)
To: xinhui pan, amd-gfx; +Cc: alexander.deucher, lijo.lazar
NAK, that is exactly what we try to avoid here.
Christian.
Am 13.09.21 um 07:55 schrieb xinhui pan:
> This is used for direct IB submission to ring.
>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index ab2351ba9574..f97a28a49120 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -184,6 +184,8 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
> else if (ring == &adev->sdma.instance[0].page)
> sched_hw_submission = 256;
>
> + mutex_init(&ring->lock);
> +
> if (ring->adev == NULL) {
> if (adev->num_rings >= AMDGPU_MAX_RINGS)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 4d380e79752c..544766429b5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -215,6 +215,7 @@ struct amdgpu_ring {
> struct amdgpu_fence_driver fence_drv;
> struct drm_gpu_scheduler sched;
>
> + struct mutex lock;
> struct amdgpu_bo *ring_obj;
> volatile uint32_t *ring;
> unsigned rptr_offs;
^ permalink raw reply [flat|nested] 3+ messages in thread