* RFC: Removal of some mutexes from the radeon driver
@ 2012-05-11 10:10 Christian König
2012-05-11 10:10 ` [PATCH 1/5] drm/radeon: remove radeon_fence_create Christian König
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Christian König @ 2012-05-11 10:10 UTC (permalink / raw)
To: dri-devel
Hi everybody,
well the following patches remove the cs and vram mutex from the radeon driver
and so are something very experimental. The first three just move what I know
to be still critical into the protection of the ring mutex, and the other two
actually remove the mutexs.
Interestingly it still survives a couple of hours running the following script:
while true
do
for (( x = 10, y = 45, i = 0; $i < 144; i = $i + 1 ))
do
glxgears -geometry 100x100+$x+$y > /dev/null 2>&1 &
x=$(expr $x + 120)
if [ $x -gt 1920 ]
then
x=10
y=$(expr $y + 110)
fi
done
sleep 30
killall glxgears
done
So the mutexes doesn't seem to protected something so critical and I'm
wondering why we still have them so widely locked.
Cheers,
Christian.
PS: I'm away for the next week or so don't expect any response soon.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/5] drm/radeon: remove radeon_fence_create
2012-05-11 10:10 RFC: Removal of some mutexes from the radeon driver Christian König
@ 2012-05-11 10:10 ` Christian König
2012-05-11 10:10 ` [PATCH 2/5] drm/radeon: add infrastructure for advanced ring synchronization Christian König
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2012-05-11 10:10 UTC (permalink / raw)
To: dri-devel
It is completely unnecessary to create fences
before they are emitted, so remove it and a bunch
of checks if fences are emitted or not.
Signed-off-by: Christian König <deathsimple@vodafone.de>
---
drivers/gpu/drm/radeon/evergreen.c | 2 +-
drivers/gpu/drm/radeon/ni.c | 2 +-
drivers/gpu/drm/radeon/r100.c | 4 +-
drivers/gpu/drm/radeon/r200.c | 4 +-
drivers/gpu/drm/radeon/r600.c | 4 +-
drivers/gpu/drm/radeon/r600_blit_kms.c | 6 +--
drivers/gpu/drm/radeon/radeon.h | 11 +++--
drivers/gpu/drm/radeon/radeon_asic.h | 8 ++--
drivers/gpu/drm/radeon/radeon_benchmark.c | 10 +----
drivers/gpu/drm/radeon/radeon_fence.c | 42 ++++++------------
drivers/gpu/drm/radeon/radeon_ring.c | 19 +++++----
drivers/gpu/drm/radeon/radeon_sa.c | 2 +-
drivers/gpu/drm/radeon/radeon_test.c | 66 ++++++++++++-----------------
drivers/gpu/drm/radeon/radeon_ttm.c | 30 +++++--------
drivers/gpu/drm/radeon/si.c | 6 +--
15 files changed, 86 insertions(+), 130 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 7e7ac3d..81678be 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1366,7 +1366,7 @@ void evergreen_mc_program(struct radeon_device *rdev)
*/
void evergreen_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib)
{
- struct radeon_ring *ring = &rdev->ring[ib->fence->ring];
+ struct radeon_ring *ring = &rdev->ring[ib->ring];
/* set to DX10/11 mode */
radeon_ring_write(ring, PACKET3(PACKET3_MODE_CONTROL, 0));
diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
index 107b217..4c2758d 100644
--- a/drivers/gpu/drm/radeon/ni.c
+++ b/drivers/gpu/drm/radeon/ni.c
@@ -1127,7 +1127,7 @@ void cayman_fence_ring_emit(struct radeon_device *rdev,
void cayman_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib)
{
- struct radeon_ring *ring = &rdev->ring[ib->fence->ring];
+ struct radeon_ring *ring = &rdev->ring[ib->ring];
/* set to DX10/11 mode */
radeon_ring_write(ring, PACKET3(PACKET3_MODE_CONTROL, 0));
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
index 0874a6d..1e3518d 100644
--- a/drivers/gpu/drm/radeon/r100.c
+++ b/drivers/gpu/drm/radeon/r100.c
@@ -883,7 +883,7 @@ int r100_copy_blit(struct radeon_device *rdev,
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence)
+ struct radeon_fence **fence)
{
struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
uint32_t cur_pages;
@@ -947,7 +947,7 @@ int r100_copy_blit(struct radeon_device *rdev,
RADEON_WAIT_HOST_IDLECLEAN |
RADEON_WAIT_DMA_GUI_IDLE);
if (fence) {
- r = radeon_fence_emit(rdev, fence);
+ r = radeon_fence_emit(rdev, fence, RADEON_RING_TYPE_GFX_INDEX);
}
radeon_ring_unlock_commit(rdev, ring);
return r;
diff --git a/drivers/gpu/drm/radeon/r200.c b/drivers/gpu/drm/radeon/r200.c
index a26144d..f088925 100644
--- a/drivers/gpu/drm/radeon/r200.c
+++ b/drivers/gpu/drm/radeon/r200.c
@@ -85,7 +85,7 @@ int r200_copy_dma(struct radeon_device *rdev,
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence)
+ struct radeon_fence **fence)
{
struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
uint32_t size;
@@ -120,7 +120,7 @@ int r200_copy_dma(struct radeon_device *rdev,
radeon_ring_write(ring, PACKET0(RADEON_WAIT_UNTIL, 0));
radeon_ring_write(ring, RADEON_WAIT_DMA_GUI_IDLE);
if (fence) {
- r = radeon_fence_emit(rdev, fence);
+ r = radeon_fence_emit(rdev, fence, RADEON_RING_TYPE_GFX_INDEX);
}
radeon_ring_unlock_commit(rdev, ring);
return r;
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index 4c0d8c9..b680cc4 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -2361,7 +2361,7 @@ int r600_copy_blit(struct radeon_device *rdev,
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence)
+ struct radeon_fence **fence)
{
struct radeon_sa_bo *vb = NULL;
int r;
@@ -2666,7 +2666,7 @@ void r600_fini(struct radeon_device *rdev)
*/
void r600_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib)
{
- struct radeon_ring *ring = &rdev->ring[ib->fence->ring];
+ struct radeon_ring *ring = &rdev->ring[ib->ring];
/* FIXME: implement */
radeon_ring_write(ring, PACKET3(PACKET3_INDIRECT_BUFFER, 2));
diff --git a/drivers/gpu/drm/radeon/r600_blit_kms.c b/drivers/gpu/drm/radeon/r600_blit_kms.c
index ef20822..5d1f1c0 100644
--- a/drivers/gpu/drm/radeon/r600_blit_kms.c
+++ b/drivers/gpu/drm/radeon/r600_blit_kms.c
@@ -703,20 +703,20 @@ int r600_blit_prepare_copy(struct radeon_device *rdev, unsigned num_gpu_pages,
return 0;
}
-void r600_blit_done_copy(struct radeon_device *rdev, struct radeon_fence *fence,
+void r600_blit_done_copy(struct radeon_device *rdev, struct radeon_fence **fence,
struct radeon_sa_bo *vb)
{
struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
int r;
- r = radeon_fence_emit(rdev, fence);
+ r = radeon_fence_emit(rdev, fence, RADEON_RING_TYPE_GFX_INDEX);
if (r) {
radeon_ring_unlock_undo(rdev, ring);
return;
}
radeon_ring_unlock_commit(rdev, ring);
- radeon_sa_bo_free(rdev, &vb, fence);
+ radeon_sa_bo_free(rdev, &vb, *fence);
}
void r600_kms_blit_copy(struct radeon_device *rdev,
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 60233d7..cd2d10f 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -113,7 +113,6 @@ extern int radeon_lockup_timeout;
/* fence seq are set to this number when signaled */
#define RADEON_FENCE_SIGNALED_SEQ 0LL
-#define RADEON_FENCE_NOTEMITED_SEQ (~0LL)
/* internal ring indices */
/* r1xx+ has gfx CP ring */
@@ -277,8 +276,7 @@ struct radeon_fence {
int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring);
int radeon_fence_driver_init(struct radeon_device *rdev);
void radeon_fence_driver_fini(struct radeon_device *rdev);
-int radeon_fence_create(struct radeon_device *rdev, struct radeon_fence **fence, int ring);
-int radeon_fence_emit(struct radeon_device *rdev, struct radeon_fence *fence);
+int radeon_fence_emit(struct radeon_device *rdev, struct radeon_fence **fence, int ring);
void radeon_fence_process(struct radeon_device *rdev, int ring);
bool radeon_fence_signaled(struct radeon_fence *fence);
int radeon_fence_wait(struct radeon_fence *fence, bool interruptible);
@@ -627,6 +625,7 @@ struct radeon_ib {
uint32_t length_dw;
uint64_t gpu_addr;
uint32_t *ptr;
+ int ring;
struct radeon_fence *fence;
unsigned vm_id;
bool is_const_ib;
@@ -1191,20 +1190,20 @@ struct radeon_asic {
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence);
+ struct radeon_fence **fence);
u32 blit_ring_index;
int (*dma)(struct radeon_device *rdev,
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence);
+ struct radeon_fence **fence);
u32 dma_ring_index;
/* method used for bo copy */
int (*copy)(struct radeon_device *rdev,
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence);
+ struct radeon_fence **fence);
/* ring used for bo copies */
u32 copy_ring_index;
} copy;
diff --git a/drivers/gpu/drm/radeon/radeon_asic.h b/drivers/gpu/drm/radeon/radeon_asic.h
index 05a4e15..6d0beaf 100644
--- a/drivers/gpu/drm/radeon/radeon_asic.h
+++ b/drivers/gpu/drm/radeon/radeon_asic.h
@@ -79,7 +79,7 @@ int r100_copy_blit(struct radeon_device *rdev,
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence);
+ struct radeon_fence **fence);
int r100_set_surface_reg(struct radeon_device *rdev, int reg,
uint32_t tiling_flags, uint32_t pitch,
uint32_t offset, uint32_t obj_size);
@@ -144,7 +144,7 @@ extern int r200_copy_dma(struct radeon_device *rdev,
uint64_t src_offset,
uint64_t dst_offset,
unsigned num_gpu_pages,
- struct radeon_fence *fence);
+ struct radeon_fence **fence);
void r200_set_safe_registers(struct radeon_device *rdev);
/*
@@ -318,7 +318,7 @@ void r600_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib);
int r600_ring_test(struct radeon_device *rdev, struct radeon_ring *cp);
int r600_copy_blit(struct radeon_device *rdev,
uint64_t src_offset, uint64_t dst_offset,
- unsigned num_gpu_pages, struct radeon_fence *fence);
+ unsigned num_gpu_pages, struct radeon_fence **fence);
void r600_hpd_init(struct radeon_device *rdev);
void r600_hpd_fini(struct radeon_device *rdev);
bool r600_hpd_sense(struct radeon_device *rdev, enum radeon_hpd_id hpd);
@@ -370,7 +370,7 @@ void r600_hdmi_update_audio_settings(struct drm_encoder *encoder);
/* r600 blit */
int r600_blit_prepare_copy(struct radeon_device *rdev, unsigned num_gpu_pages,
struct radeon_sa_bo **vb);
-void r600_blit_done_copy(struct radeon_device *rdev, struct radeon_fence *fence,
+void r600_blit_done_copy(struct radeon_device *rdev, struct radeon_fence **fence,
struct radeon_sa_bo *vb);
void r600_kms_blit_copy(struct radeon_device *rdev,
u64 src_gpu_addr, u64 dst_gpu_addr,
diff --git a/drivers/gpu/drm/radeon/radeon_benchmark.c b/drivers/gpu/drm/radeon/radeon_benchmark.c
index fef7b72..61019e7 100644
--- a/drivers/gpu/drm/radeon/radeon_benchmark.c
+++ b/drivers/gpu/drm/radeon/radeon_benchmark.c
@@ -45,20 +45,14 @@ static int radeon_benchmark_do_move(struct radeon_device *rdev, unsigned size,
for (i = 0; i < n; i++) {
switch (flag) {
case RADEON_BENCHMARK_COPY_DMA:
- r = radeon_fence_create(rdev, &fence, radeon_copy_dma_ring_index(rdev));
- if (r)
- return r;
r = radeon_copy_dma(rdev, saddr, daddr,
size / RADEON_GPU_PAGE_SIZE,
- fence);
+ &fence);
break;
case RADEON_BENCHMARK_COPY_BLIT:
- r = radeon_fence_create(rdev, &fence, radeon_copy_blit_ring_index(rdev));
- if (r)
- return r;
r = radeon_copy_blit(rdev, saddr, daddr,
size / RADEON_GPU_PAGE_SIZE,
- fence);
+ &fence);
break;
default:
DRM_ERROR("Unknown copy method\n");
diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
index 11f5f40..401d346 100644
--- a/drivers/gpu/drm/radeon/radeon_fence.c
+++ b/drivers/gpu/drm/radeon/radeon_fence.c
@@ -61,15 +61,21 @@ static u32 radeon_fence_read(struct radeon_device *rdev, int ring)
return seq;
}
-int radeon_fence_emit(struct radeon_device *rdev, struct radeon_fence *fence)
+int radeon_fence_emit(struct radeon_device *rdev,
+ struct radeon_fence **fence,
+ int ring)
{
/* we are protected by the ring emission mutex */
- if (fence->seq && fence->seq < RADEON_FENCE_NOTEMITED_SEQ) {
- return 0;
+ *fence = kmalloc(sizeof(struct radeon_fence), GFP_KERNEL);
+ if ((*fence) == NULL) {
+ return -ENOMEM;
}
- fence->seq = ++rdev->fence_drv[fence->ring].seq;
- radeon_fence_ring_emit(rdev, fence->ring, fence);
- trace_radeon_fence_emit(rdev->ddev, fence->seq);
+ kref_init(&((*fence)->kref));
+ (*fence)->rdev = rdev;
+ (*fence)->seq = ++rdev->fence_drv[ring].seq;
+ (*fence)->ring = ring;
+ radeon_fence_ring_emit(rdev, ring, *fence);
+ trace_radeon_fence_emit(rdev->ddev, (*fence)->seq);
return 0;
}
@@ -138,25 +144,9 @@ static void radeon_fence_destroy(struct kref *kref)
struct radeon_fence *fence;
fence = container_of(kref, struct radeon_fence, kref);
- fence->seq = RADEON_FENCE_NOTEMITED_SEQ;
kfree(fence);
}
-int radeon_fence_create(struct radeon_device *rdev,
- struct radeon_fence **fence,
- int ring)
-{
- *fence = kmalloc(sizeof(struct radeon_fence), GFP_KERNEL);
- if ((*fence) == NULL) {
- return -ENOMEM;
- }
- kref_init(&((*fence)->kref));
- (*fence)->rdev = rdev;
- (*fence)->seq = RADEON_FENCE_NOTEMITED_SEQ;
- (*fence)->ring = ring;
- return 0;
-}
-
static bool radeon_fence_seq_signaled(struct radeon_device *rdev,
u64 seq, unsigned ring)
{
@@ -176,10 +166,6 @@ bool radeon_fence_signaled(struct radeon_fence *fence)
if (!fence) {
return true;
}
- if (fence->seq == RADEON_FENCE_NOTEMITED_SEQ) {
- WARN(1, "Querying an unemitted fence : %p !\n", fence);
- return true;
- }
if (fence->seq == RADEON_FENCE_SIGNALED_SEQ) {
return true;
}
@@ -444,9 +430,7 @@ int radeon_fence_wait_any(struct radeon_device *rdev,
return 0;
}
- if (fences[i]->seq < RADEON_FENCE_NOTEMITED_SEQ) {
- seq[i] = fences[i]->seq;
- }
+ seq[i] = fences[i]->seq;
}
r = radeon_fence_wait_any_seq(rdev, seq, intr);
diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
index a5dee76..2d91bbb 100644
--- a/drivers/gpu/drm/radeon/radeon_ring.c
+++ b/drivers/gpu/drm/radeon/radeon_ring.c
@@ -74,13 +74,9 @@ int radeon_ib_get(struct radeon_device *rdev, int ring,
dev_err(rdev->dev, "failed to get a new IB (%d)\n", r);
return r;
}
- r = radeon_fence_create(rdev, &ib->fence, ring);
- if (r) {
- dev_err(rdev->dev, "failed to create fence for new IB (%d)\n", r);
- radeon_sa_bo_free(rdev, &ib->sa_bo, NULL);
- return r;
- }
+ ib->ring = ring;
+ ib->fence = NULL;
ib->ptr = radeon_sa_bo_cpu_addr(ib->sa_bo);
ib->gpu_addr = radeon_sa_bo_gpu_addr(ib->sa_bo);
ib->vm_id = 0;
@@ -99,7 +95,7 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib)
int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
{
- struct radeon_ring *ring = &rdev->ring[ib->fence->ring];
+ struct radeon_ring *ring = &rdev->ring[ib->ring];
int r = 0;
if (!ib->length_dw || !ring->ready) {
@@ -114,8 +110,13 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
dev_err(rdev->dev, "scheduling IB failed (%d).\n", r);
return r;
}
- radeon_ring_ib_execute(rdev, ib->fence->ring, ib);
- radeon_fence_emit(rdev, ib->fence);
+ radeon_ring_ib_execute(rdev, ib->ring, ib);
+ r = radeon_fence_emit(rdev, &ib->fence, ib->ring);
+ if (r) {
+ dev_err(rdev->dev, "failed to emit fence for new IB (%d)\n", r);
+ radeon_ring_unlock_undo(rdev, ring);
+ return r;
+ }
radeon_ring_unlock_commit(rdev, ring);
return 0;
}
diff --git a/drivers/gpu/drm/radeon/radeon_sa.c b/drivers/gpu/drm/radeon/radeon_sa.c
index c3ac7f4..0619ee2 100644
--- a/drivers/gpu/drm/radeon/radeon_sa.c
+++ b/drivers/gpu/drm/radeon/radeon_sa.c
@@ -349,7 +349,7 @@ void radeon_sa_bo_free(struct radeon_device *rdev, struct radeon_sa_bo **sa_bo,
sa_manager = (*sa_bo)->manager;
spin_lock(&sa_manager->lock);
- if (fence && fence->seq && fence->seq < RADEON_FENCE_NOTEMITED_SEQ) {
+ if (fence && !radeon_fence_signaled(fence)) {
(*sa_bo)->fence = radeon_fence_ref(fence);
list_add_tail(&(*sa_bo)->flist,
&sa_manager->flist[fence->ring]);
diff --git a/drivers/gpu/drm/radeon/radeon_test.c b/drivers/gpu/drm/radeon/radeon_test.c
index b057387..eace3a5 100644
--- a/drivers/gpu/drm/radeon/radeon_test.c
+++ b/drivers/gpu/drm/radeon/radeon_test.c
@@ -106,13 +106,7 @@ void radeon_test_moves(struct radeon_device *rdev)
radeon_bo_kunmap(gtt_obj[i]);
- r = radeon_fence_create(rdev, &fence, RADEON_RING_TYPE_GFX_INDEX);
- if (r) {
- DRM_ERROR("Failed to create GTT->VRAM fence %d\n", i);
- goto out_cleanup;
- }
-
- r = radeon_copy(rdev, gtt_addr, vram_addr, size / RADEON_GPU_PAGE_SIZE, fence);
+ r = radeon_copy(rdev, gtt_addr, vram_addr, size / RADEON_GPU_PAGE_SIZE, &fence);
if (r) {
DRM_ERROR("Failed GTT->VRAM copy %d\n", i);
goto out_cleanup;
@@ -155,13 +149,7 @@ void radeon_test_moves(struct radeon_device *rdev)
radeon_bo_kunmap(vram_obj);
- r = radeon_fence_create(rdev, &fence, RADEON_RING_TYPE_GFX_INDEX);
- if (r) {
- DRM_ERROR("Failed to create VRAM->GTT fence %d\n", i);
- goto out_cleanup;
- }
-
- r = radeon_copy(rdev, vram_addr, gtt_addr, size / RADEON_GPU_PAGE_SIZE, fence);
+ r = radeon_copy(rdev, vram_addr, gtt_addr, size / RADEON_GPU_PAGE_SIZE, &fence);
if (r) {
DRM_ERROR("Failed VRAM->GTT copy %d\n", i);
goto out_cleanup;
@@ -245,17 +233,6 @@ void radeon_test_ring_sync(struct radeon_device *rdev,
int ridxB = radeon_ring_index(rdev, ringB);
int r;
- r = radeon_fence_create(rdev, &fence1, ridxA);
- if (r) {
- DRM_ERROR("Failed to create sync fence 1\n");
- goto out_cleanup;
- }
- r = radeon_fence_create(rdev, &fence2, ridxA);
- if (r) {
- DRM_ERROR("Failed to create sync fence 2\n");
- goto out_cleanup;
- }
-
r = radeon_semaphore_create(rdev, &semaphore);
if (r) {
DRM_ERROR("Failed to create semaphore\n");
@@ -268,9 +245,19 @@ void radeon_test_ring_sync(struct radeon_device *rdev,
goto out_cleanup;
}
radeon_semaphore_emit_wait(rdev, ridxA, semaphore);
- radeon_fence_emit(rdev, fence1);
+ r = radeon_fence_emit(rdev, &fence1, ridxA);
+ if (r) {
+ DRM_ERROR("Failed to emit fence 1\n");
+ radeon_ring_unlock_undo(rdev, ringA);
+ goto out_cleanup;
+ }
radeon_semaphore_emit_wait(rdev, ridxA, semaphore);
- radeon_fence_emit(rdev, fence2);
+ r = radeon_fence_emit(rdev, &fence2, ridxA);
+ if (r) {
+ DRM_ERROR("Failed to emit fence 2\n");
+ radeon_ring_unlock_undo(rdev, ringA);
+ goto out_cleanup;
+ }
radeon_ring_unlock_commit(rdev, ringA);
mdelay(1000);
@@ -342,17 +329,6 @@ void radeon_test_ring_sync2(struct radeon_device *rdev,
bool sigA, sigB;
int i, r;
- r = radeon_fence_create(rdev, &fenceA, ridxA);
- if (r) {
- DRM_ERROR("Failed to create sync fence 1\n");
- goto out_cleanup;
- }
- r = radeon_fence_create(rdev, &fenceB, ridxB);
- if (r) {
- DRM_ERROR("Failed to create sync fence 2\n");
- goto out_cleanup;
- }
-
r = radeon_semaphore_create(rdev, &semaphore);
if (r) {
DRM_ERROR("Failed to create semaphore\n");
@@ -365,7 +341,12 @@ void radeon_test_ring_sync2(struct radeon_device *rdev,
goto out_cleanup;
}
radeon_semaphore_emit_wait(rdev, ridxA, semaphore);
- radeon_fence_emit(rdev, fenceA);
+ r = radeon_fence_emit(rdev, &fenceA, ridxA);
+ if (r) {
+ DRM_ERROR("Failed to emit sync fence 1\n");
+ radeon_ring_unlock_undo(rdev, ringA);
+ goto out_cleanup;
+ }
radeon_ring_unlock_commit(rdev, ringA);
r = radeon_ring_lock(rdev, ringB, 64);
@@ -374,7 +355,12 @@ void radeon_test_ring_sync2(struct radeon_device *rdev,
goto out_cleanup;
}
radeon_semaphore_emit_wait(rdev, ridxB, semaphore);
- radeon_fence_emit(rdev, fenceB);
+ r = radeon_fence_emit(rdev, &fenceB, ridxB);
+ if (r) {
+ DRM_ERROR("Failed to create sync fence 2\n");
+ radeon_ring_unlock_undo(rdev, ringB);
+ goto out_cleanup;
+ }
radeon_ring_unlock_commit(rdev, ringB);
mdelay(1000);
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 0f6aee8..860e730 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -222,15 +222,12 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
{
struct radeon_device *rdev;
uint64_t old_start, new_start;
- struct radeon_fence *fence, *old_fence;
+ struct radeon_fence *fence;
struct radeon_semaphore *sem = NULL;
- int r;
+ int r, ridx;
rdev = radeon_get_rdev(bo->bdev);
- r = radeon_fence_create(rdev, &fence, radeon_copy_ring_index(rdev));
- if (unlikely(r)) {
- return r;
- }
+ ridx = radeon_copy_ring_index(rdev);
old_start = old_mem->start << PAGE_SHIFT;
new_start = new_mem->start << PAGE_SHIFT;
@@ -243,7 +240,6 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
break;
default:
DRM_ERROR("Unknown placement %d\n", old_mem->mem_type);
- radeon_fence_unref(&fence);
return -EINVAL;
}
switch (new_mem->mem_type) {
@@ -255,42 +251,38 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
break;
default:
DRM_ERROR("Unknown placement %d\n", old_mem->mem_type);
- radeon_fence_unref(&fence);
return -EINVAL;
}
- if (!rdev->ring[radeon_copy_ring_index(rdev)].ready) {
+ if (!rdev->ring[ridx].ready) {
DRM_ERROR("Trying to move memory with ring turned off.\n");
- radeon_fence_unref(&fence);
return -EINVAL;
}
BUILD_BUG_ON((PAGE_SIZE % RADEON_GPU_PAGE_SIZE) != 0);
/* sync other rings */
- old_fence = bo->sync_obj;
- if (old_fence && old_fence->ring != fence->ring
- && !radeon_fence_signaled(old_fence)) {
+ fence = bo->sync_obj;
+ if (fence && fence->ring != ridx
+ && !radeon_fence_signaled(fence)) {
bool sync_to_ring[RADEON_NUM_RINGS] = { };
- sync_to_ring[old_fence->ring] = true;
+ sync_to_ring[fence->ring] = true;
r = radeon_semaphore_create(rdev, &sem);
if (r) {
- radeon_fence_unref(&fence);
return r;
}
- r = radeon_semaphore_sync_rings(rdev, sem,
- sync_to_ring, fence->ring);
+ r = radeon_semaphore_sync_rings(rdev, sem, sync_to_ring, ridx);
if (r) {
radeon_semaphore_free(rdev, sem, NULL);
- radeon_fence_unref(&fence);
return r;
}
}
+ fence = NULL;
r = radeon_copy(rdev, old_start, new_start,
new_mem->num_pages * (PAGE_SIZE / RADEON_GPU_PAGE_SIZE), /* GPU pages */
- fence);
+ &fence);
/* FIXME: handle copy error */
r = ttm_bo_move_accel_cleanup(bo, (void *)fence, NULL,
evict, no_wait_reserve, no_wait_gpu, new_mem);
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index be3b9fd..099dae4 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -1915,7 +1915,7 @@ void si_fence_ring_emit(struct radeon_device *rdev,
*/
void si_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib)
{
- struct radeon_ring *ring = &rdev->ring[ib->fence->ring];
+ struct radeon_ring *ring = &rdev->ring[ib->ring];
u32 header;
if (ib->is_const_ib)
@@ -2854,7 +2854,7 @@ int si_ib_parse(struct radeon_device *rdev, struct radeon_ib *ib)
if (ib->is_const_ib)
ret = si_vm_packet3_ce_check(rdev, ib->ptr, &pkt);
else {
- switch (ib->fence->ring) {
+ switch (ib->ring) {
case RADEON_RING_TYPE_GFX_INDEX:
ret = si_vm_packet3_gfx_check(rdev, ib->ptr, &pkt);
break;
@@ -2863,7 +2863,7 @@ int si_ib_parse(struct radeon_device *rdev, struct radeon_ib *ib)
ret = si_vm_packet3_compute_check(rdev, ib->ptr, &pkt);
break;
default:
- dev_err(rdev->dev, "Non-PM4 ring %d !\n", ib->fence->ring);
+ dev_err(rdev->dev, "Non-PM4 ring %d !\n", ib->ring);
ret = -EINVAL;
break;
}
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/5] drm/radeon: add infrastructure for advanced ring synchronization
2012-05-11 10:10 RFC: Removal of some mutexes from the radeon driver Christian König
2012-05-11 10:10 ` [PATCH 1/5] drm/radeon: remove radeon_fence_create Christian König
@ 2012-05-11 10:10 ` Christian König
2012-05-11 10:10 ` [PATCH 3/5] drm/radeon: rework ring syncing code Christian König
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2012-05-11 10:10 UTC (permalink / raw)
To: dri-devel
Signed-off-by: Christian König <deathsimple@vodafone.de>
---
drivers/gpu/drm/radeon/radeon.h | 23 ++++++++++-
drivers/gpu/drm/radeon/radeon_fence.c | 73 +++++++++++++++++++++++++++++----
2 files changed, 85 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index cd2d10f..b02e36b 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -257,8 +257,8 @@ struct radeon_fence_driver {
uint32_t scratch_reg;
uint64_t gpu_addr;
volatile uint32_t *cpu_addr;
- /* seq is protected by ring emission lock */
- uint64_t seq;
+ /* sync_seq is protected by ring emission lock */
+ uint64_t sync_seq[RADEON_NUM_RINGS];
atomic64_t last_seq;
unsigned long last_activity;
bool initialized;
@@ -288,6 +288,25 @@ int radeon_fence_wait_any(struct radeon_device *rdev,
struct radeon_fence *radeon_fence_ref(struct radeon_fence *fence);
void radeon_fence_unref(struct radeon_fence **fence);
unsigned radeon_fence_count_emitted(struct radeon_device *rdev, int ring);
+bool radeon_fence_need_sync(struct radeon_fence *fence, int ring);
+void radeon_fence_note_sync(struct radeon_fence *fence, int ring);
+static inline struct radeon_fence *radeon_fence_later(struct radeon_fence *a,
+ struct radeon_fence *b)
+{
+ if (!a) {
+ return b;
+ }
+
+ if (!b) {
+ return a;
+ }
+
+ if (a->seq > b->seq) {
+ return a;
+ } else {
+ return b;
+ }
+}
/*
* Tiling registers
diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
index 401d346..7b55625 100644
--- a/drivers/gpu/drm/radeon/radeon_fence.c
+++ b/drivers/gpu/drm/radeon/radeon_fence.c
@@ -72,7 +72,7 @@ int radeon_fence_emit(struct radeon_device *rdev,
}
kref_init(&((*fence)->kref));
(*fence)->rdev = rdev;
- (*fence)->seq = ++rdev->fence_drv[ring].seq;
+ (*fence)->seq = ++rdev->fence_drv[ring].sync_seq[ring];
(*fence)->ring = ring;
radeon_fence_ring_emit(rdev, ring, *fence);
trace_radeon_fence_emit(rdev->ddev, (*fence)->seq);
@@ -449,7 +449,7 @@ int radeon_fence_wait_next_locked(struct radeon_device *rdev, int ring)
* wait.
*/
seq = atomic64_read(&rdev->fence_drv[ring].last_seq) + 1ULL;
- if (seq >= rdev->fence_drv[ring].seq) {
+ if (seq >= rdev->fence_drv[ring].sync_seq[ring]) {
/* nothing to wait for, last_seq is
already the last emited fence */
return -ENOENT;
@@ -464,7 +464,7 @@ int radeon_fence_wait_empty_locked(struct radeon_device *rdev, int ring)
* activity can be scheduled so there won't be concurrent access
* to seq value.
*/
- return radeon_fence_wait_seq(rdev, rdev->fence_drv[ring].seq,
+ return radeon_fence_wait_seq(rdev, rdev->fence_drv[ring].sync_seq[ring],
ring, false, false);
}
@@ -492,7 +492,8 @@ unsigned radeon_fence_count_emitted(struct radeon_device *rdev, int ring)
* but it's ok to report slightly wrong fence count here.
*/
radeon_fence_process(rdev, ring);
- emitted = rdev->fence_drv[ring].seq - atomic64_read(&rdev->fence_drv[ring].last_seq);
+ emitted = rdev->fence_drv[ring].sync_seq[ring]
+ - atomic64_read(&rdev->fence_drv[ring].last_seq);
/* to avoid 32bits warp around */
if (emitted > 0x10000000) {
emitted = 0x10000000;
@@ -500,6 +501,51 @@ unsigned radeon_fence_count_emitted(struct radeon_device *rdev, int ring)
return (unsigned)emitted;
}
+bool radeon_fence_need_sync(struct radeon_fence *fence, int dst_ring)
+{
+ struct radeon_fence_driver *fdrv;
+
+ if (!fence) {
+ return false;
+ }
+
+ if (fence->ring == dst_ring) {
+ return false;
+ }
+
+ /* we are protected by the ring mutex */
+ fdrv = &fence->rdev->fence_drv[dst_ring];
+ if (fence->seq <= fdrv->sync_seq[fence->ring]) {
+ return false;
+ }
+
+ return true;
+}
+
+void radeon_fence_note_sync(struct radeon_fence *fence, int dst_ring)
+{
+ struct radeon_fence_driver *dst, *src;
+ unsigned i;
+
+ if (!fence) {
+ return;
+ }
+
+ if (fence->ring == dst_ring) {
+ return;
+ }
+
+ /* we are protected by the ring mutex */
+ src = &fence->rdev->fence_drv[fence->ring];
+ dst = &fence->rdev->fence_drv[dst_ring];
+ for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+ if (i == dst_ring) {
+ continue;
+ }
+ dst->sync_seq[i] = max(dst->sync_seq[i], src->sync_seq[i]);
+ }
+}
+
int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring)
{
uint64_t index;
@@ -521,7 +567,7 @@ int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring)
}
rdev->fence_drv[ring].cpu_addr = &rdev->wb.wb[index/4];
rdev->fence_drv[ring].gpu_addr = rdev->wb.gpu_addr + index;
- radeon_fence_write(rdev, rdev->fence_drv[ring].seq, ring);
+ radeon_fence_write(rdev, rdev->fence_drv[ring].sync_seq[ring], ring);
rdev->fence_drv[ring].initialized = true;
dev_info(rdev->dev, "fence driver on ring %d use gpu addr 0x%016llx and cpu addr 0x%p\n",
ring, rdev->fence_drv[ring].gpu_addr, rdev->fence_drv[ring].cpu_addr);
@@ -530,10 +576,13 @@ int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring)
static void radeon_fence_driver_init_ring(struct radeon_device *rdev, int ring)
{
+ int i;
+
rdev->fence_drv[ring].scratch_reg = -1;
rdev->fence_drv[ring].cpu_addr = NULL;
rdev->fence_drv[ring].gpu_addr = 0;
- rdev->fence_drv[ring].seq = 0;
+ for (i = 0; i < RADEON_NUM_RINGS; ++i)
+ rdev->fence_drv[ring].sync_seq[i] = 0;
atomic64_set(&rdev->fence_drv[ring].last_seq, 0);
rdev->fence_drv[ring].last_activity = jiffies;
rdev->fence_drv[ring].initialized = false;
@@ -579,7 +628,7 @@ static int radeon_debugfs_fence_info(struct seq_file *m, void *data)
struct drm_info_node *node = (struct drm_info_node *)m->private;
struct drm_device *dev = node->minor->dev;
struct radeon_device *rdev = dev->dev_private;
- int i;
+ int i, j;
for (i = 0; i < RADEON_NUM_RINGS; ++i) {
if (!rdev->fence_drv[i].initialized)
@@ -588,8 +637,14 @@ static int radeon_debugfs_fence_info(struct seq_file *m, void *data)
seq_printf(m, "--- ring %d ---\n", i);
seq_printf(m, "Last signaled fence 0x%016llx\n",
(unsigned long long)atomic64_read(&rdev->fence_drv[i].last_seq));
- seq_printf(m, "Last emitted 0x%016llx\n",
- rdev->fence_drv[i].seq);
+ seq_printf(m, "Last emitted 0x%016llx\n",
+ rdev->fence_drv[i].sync_seq[i]);
+
+ for (j = 0; j < RADEON_NUM_RINGS; ++j) {
+ if (i != j && rdev->fence_drv[j].initialized)
+ seq_printf(m, "Last sync to ring %d 0x%016llx\n",
+ j, rdev->fence_drv[i].sync_seq[j]);
+ }
}
return 0;
}
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/5] drm/radeon: rework ring syncing code
2012-05-11 10:10 RFC: Removal of some mutexes from the radeon driver Christian König
2012-05-11 10:10 ` [PATCH 1/5] drm/radeon: remove radeon_fence_create Christian König
2012-05-11 10:10 ` [PATCH 2/5] drm/radeon: add infrastructure for advanced ring synchronization Christian König
@ 2012-05-11 10:10 ` Christian König
2012-05-11 10:10 ` [PATCH 4/5] drm/radeon: WIP remove cs_mutex Christian König
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2012-05-11 10:10 UTC (permalink / raw)
To: dri-devel
Move inter ring syncing with semaphores into the
existing ring allocations, with that we need to
lock the ring mutex only once.
Signed-off-by: Christian König <deathsimple@vodafone.de>
---
drivers/gpu/drm/radeon/evergreen_blit_kms.c | 3 +-
drivers/gpu/drm/radeon/r600.c | 5 +-
drivers/gpu/drm/radeon/r600_blit_kms.c | 24 +++++++--
drivers/gpu/drm/radeon/radeon.h | 6 +--
drivers/gpu/drm/radeon/radeon_asic.h | 5 +-
drivers/gpu/drm/radeon/radeon_cs.c | 38 +++-----------
drivers/gpu/drm/radeon/radeon_ring.c | 30 +++++++++--
drivers/gpu/drm/radeon/radeon_semaphore.c | 71 ++++++++++-----------------
drivers/gpu/drm/radeon/radeon_test.c | 6 +--
drivers/gpu/drm/radeon/radeon_ttm.c | 20 --------
10 files changed, 92 insertions(+), 116 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen_blit_kms.c b/drivers/gpu/drm/radeon/evergreen_blit_kms.c
index 30f0480..4bbf83c 100644
--- a/drivers/gpu/drm/radeon/evergreen_blit_kms.c
+++ b/drivers/gpu/drm/radeon/evergreen_blit_kms.c
@@ -622,7 +622,8 @@ int evergreen_blit_init(struct radeon_device *rdev)
rdev->r600_blit.primitives.draw_auto = draw_auto;
rdev->r600_blit.primitives.set_default_state = set_default_state;
- rdev->r600_blit.ring_size_common = 55; /* shaders + def state */
+ rdev->r600_blit.ring_size_common = 8; /* sync semaphore */
+ rdev->r600_blit.ring_size_common += 55; /* shaders + def state */
rdev->r600_blit.ring_size_common += 16; /* fence emit for VB IB */
rdev->r600_blit.ring_size_common += 5; /* done copy */
rdev->r600_blit.ring_size_common += 16; /* fence emit for done copy */
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index b680cc4..b12687b 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -2363,15 +2363,16 @@ int r600_copy_blit(struct radeon_device *rdev,
unsigned num_gpu_pages,
struct radeon_fence **fence)
{
+ struct radeon_semaphore *sem = NULL;
struct radeon_sa_bo *vb = NULL;
int r;
- r = r600_blit_prepare_copy(rdev, num_gpu_pages, &vb);
+ r = r600_blit_prepare_copy(rdev, num_gpu_pages, fence, &vb, &sem);
if (r) {
return r;
}
r600_kms_blit_copy(rdev, src_offset, dst_offset, num_gpu_pages, vb);
- r600_blit_done_copy(rdev, fence, vb);
+ r600_blit_done_copy(rdev, fence, vb, sem);
return 0;
}
diff --git a/drivers/gpu/drm/radeon/r600_blit_kms.c b/drivers/gpu/drm/radeon/r600_blit_kms.c
index 5d1f1c0..b683221 100644
--- a/drivers/gpu/drm/radeon/r600_blit_kms.c
+++ b/drivers/gpu/drm/radeon/r600_blit_kms.c
@@ -512,7 +512,8 @@ int r600_blit_init(struct radeon_device *rdev)
rdev->r600_blit.primitives.draw_auto = draw_auto;
rdev->r600_blit.primitives.set_default_state = set_default_state;
- rdev->r600_blit.ring_size_common = 40; /* shaders + def state */
+ rdev->r600_blit.ring_size_common = 8; /* sync semaphore */
+ rdev->r600_blit.ring_size_common += 40; /* shaders + def state */
rdev->r600_blit.ring_size_common += 5; /* done copy */
rdev->r600_blit.ring_size_common += 16; /* fence emit for done copy */
@@ -666,7 +667,8 @@ static unsigned r600_blit_create_rect(unsigned num_gpu_pages,
int r600_blit_prepare_copy(struct radeon_device *rdev, unsigned num_gpu_pages,
- struct radeon_sa_bo **vb)
+ struct radeon_fence **fence, struct radeon_sa_bo **vb,
+ struct radeon_semaphore **sem)
{
struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
int r;
@@ -689,22 +691,37 @@ int r600_blit_prepare_copy(struct radeon_device *rdev, unsigned num_gpu_pages,
return r;
}
+ r = radeon_semaphore_create(rdev, sem);
+ if (r) {
+ radeon_sa_bo_free(rdev, vb, NULL);
+ return r;
+ }
+
/* calculate number of loops correctly */
ring_size = num_loops * dwords_per_loop;
ring_size += rdev->r600_blit.ring_size_common;
r = radeon_ring_lock(rdev, ring, ring_size);
if (r) {
radeon_sa_bo_free(rdev, vb, NULL);
+ radeon_semaphore_free(rdev, sem, NULL);
return r;
}
+ if (radeon_fence_need_sync(*fence, RADEON_RING_TYPE_GFX_INDEX)) {
+ radeon_semaphore_sync_rings(rdev, *sem, (*fence)->ring,
+ RADEON_RING_TYPE_GFX_INDEX);
+ radeon_fence_note_sync(*fence, RADEON_RING_TYPE_GFX_INDEX);
+ } else {
+ radeon_semaphore_free(rdev, sem, NULL);
+ }
+
rdev->r600_blit.primitives.set_default_state(rdev);
rdev->r600_blit.primitives.set_shaders(rdev);
return 0;
}
void r600_blit_done_copy(struct radeon_device *rdev, struct radeon_fence **fence,
- struct radeon_sa_bo *vb)
+ struct radeon_sa_bo *vb, struct radeon_semaphore *sem)
{
struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
int r;
@@ -717,6 +734,7 @@ void r600_blit_done_copy(struct radeon_device *rdev, struct radeon_fence **fence
radeon_ring_unlock_commit(rdev, ring);
radeon_sa_bo_free(rdev, &vb, *fence);
+ radeon_semaphore_free(rdev, &sem, *fence);
}
void r600_kms_blit_copy(struct radeon_device *rdev,
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index b02e36b..e00c50d 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -465,10 +465,9 @@ void radeon_semaphore_emit_wait(struct radeon_device *rdev, int ring,
struct radeon_semaphore *semaphore);
int radeon_semaphore_sync_rings(struct radeon_device *rdev,
struct radeon_semaphore *semaphore,
- bool sync_to[RADEON_NUM_RINGS],
- int dst_ring);
+ int signaler, int waiter);
void radeon_semaphore_free(struct radeon_device *rdev,
- struct radeon_semaphore *semaphore,
+ struct radeon_semaphore **semaphore,
struct radeon_fence *fence);
/*
@@ -648,6 +647,7 @@ struct radeon_ib {
struct radeon_fence *fence;
unsigned vm_id;
bool is_const_ib;
+ struct radeon_fence *sync_to[RADEON_NUM_RINGS];
struct radeon_semaphore *semaphore;
};
diff --git a/drivers/gpu/drm/radeon/radeon_asic.h b/drivers/gpu/drm/radeon/radeon_asic.h
index 6d0beaf..fe18f51 100644
--- a/drivers/gpu/drm/radeon/radeon_asic.h
+++ b/drivers/gpu/drm/radeon/radeon_asic.h
@@ -369,9 +369,10 @@ int r600_hdmi_buffer_status_changed(struct drm_encoder *encoder);
void r600_hdmi_update_audio_settings(struct drm_encoder *encoder);
/* r600 blit */
int r600_blit_prepare_copy(struct radeon_device *rdev, unsigned num_gpu_pages,
- struct radeon_sa_bo **vb);
+ struct radeon_fence **fence, struct radeon_sa_bo **vb,
+ struct radeon_semaphore **sem);
void r600_blit_done_copy(struct radeon_device *rdev, struct radeon_fence **fence,
- struct radeon_sa_bo *vb);
+ struct radeon_sa_bo *vb, struct radeon_semaphore *sem);
void r600_kms_blit_copy(struct radeon_device *rdev,
u64 src_gpu_addr, u64 dst_gpu_addr,
unsigned num_gpu_pages,
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
index c7d64a7..d295821 100644
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -115,36 +115,20 @@ static int radeon_cs_get_ring(struct radeon_cs_parser *p, u32 ring, s32 priority
return 0;
}
-static int radeon_cs_sync_rings(struct radeon_cs_parser *p)
+static void radeon_cs_sync_rings(struct radeon_cs_parser *p)
{
- bool sync_to_ring[RADEON_NUM_RINGS] = { };
- bool need_sync = false;
- int i, r;
+ int i;
for (i = 0; i < p->nrelocs; i++) {
- struct radeon_fence *fence;
+ struct radeon_fence *a, *b;
if (!p->relocs[i].robj || !p->relocs[i].robj->tbo.sync_obj)
continue;
- fence = p->relocs[i].robj->tbo.sync_obj;
- if (fence->ring != p->ring && !radeon_fence_signaled(fence)) {
- sync_to_ring[fence->ring] = true;
- need_sync = true;
- }
- }
-
- if (!need_sync) {
- return 0;
- }
-
- r = radeon_semaphore_create(p->rdev, &p->ib.semaphore);
- if (r) {
- return r;
+ a = p->relocs[i].robj->tbo.sync_obj;
+ b = p->ib.sync_to[a->ring];
+ p->ib.sync_to[a->ring] = radeon_fence_later(a, b);
}
-
- return radeon_semaphore_sync_rings(p->rdev, p->ib.semaphore,
- sync_to_ring, p->ring);
}
int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data)
@@ -365,10 +349,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev,
DRM_ERROR("Invalid command stream !\n");
return r;
}
- r = radeon_cs_sync_rings(parser);
- if (r) {
- DRM_ERROR("Failed to synchronize rings !\n");
- }
+ radeon_cs_sync_rings(parser);
parser->ib.vm_id = 0;
r = radeon_ib_schedule(rdev, &parser->ib);
if (r) {
@@ -465,10 +446,7 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev,
if (r) {
goto out;
}
- r = radeon_cs_sync_rings(parser);
- if (r) {
- DRM_ERROR("Failed to synchronize rings !\n");
- }
+ radeon_cs_sync_rings(parser);
if ((rdev->family >= CHIP_TAHITI) &&
(parser->chunk_const_ib_idx != -1)) {
diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
index 2d91bbb..cf70c2c 100644
--- a/drivers/gpu/drm/radeon/radeon_ring.c
+++ b/drivers/gpu/drm/radeon/radeon_ring.c
@@ -67,7 +67,7 @@ u32 radeon_get_ib_value(struct radeon_cs_parser *p, int idx)
int radeon_ib_get(struct radeon_device *rdev, int ring,
struct radeon_ib *ib, unsigned size)
{
- int r;
+ int i, r;
r = radeon_sa_bo_new(rdev, &rdev->ring_tmp_bo, &ib->sa_bo, size, 256, true);
if (r) {
@@ -75,20 +75,26 @@ int radeon_ib_get(struct radeon_device *rdev, int ring,
return r;
}
+ r = radeon_semaphore_create(rdev, &ib->semaphore);
+ if (r) {
+ return r;
+ }
+
ib->ring = ring;
ib->fence = NULL;
ib->ptr = radeon_sa_bo_cpu_addr(ib->sa_bo);
ib->gpu_addr = radeon_sa_bo_gpu_addr(ib->sa_bo);
ib->vm_id = 0;
ib->is_const_ib = false;
- ib->semaphore = NULL;
+ for (i = 0; i < RADEON_NUM_RINGS; ++i)
+ ib->sync_to[i] = NULL;
return 0;
}
void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib)
{
- radeon_semaphore_free(rdev, ib->semaphore, ib->fence);
+ radeon_semaphore_free(rdev, &ib->semaphore, ib->fence);
radeon_sa_bo_free(rdev, &ib->sa_bo, ib->fence);
radeon_fence_unref(&ib->fence);
}
@@ -96,7 +102,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib)
int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
{
struct radeon_ring *ring = &rdev->ring[ib->ring];
- int r = 0;
+ bool need_sync = false;
+ int i, r = 0;
if (!ib->length_dw || !ring->ready) {
/* TODO: Nothings in the ib we should report. */
@@ -105,11 +112,24 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
}
/* 64 dwords should be enough for fence too */
- r = radeon_ring_lock(rdev, ring, 64);
+ r = radeon_ring_lock(rdev, ring, 64 + RADEON_NUM_RINGS * 8);
if (r) {
dev_err(rdev->dev, "scheduling IB failed (%d).\n", r);
return r;
}
+ for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+ struct radeon_fence *fence = ib->sync_to[i];
+ if (radeon_fence_need_sync(fence, ib->ring)) {
+ need_sync = true;
+ radeon_semaphore_sync_rings(rdev, ib->semaphore,
+ fence->ring, ib->ring);
+ radeon_fence_note_sync(fence, ib->ring);
+ }
+ }
+ /* immediately free semaphore when we don't need to sync */
+ if (!need_sync) {
+ radeon_semaphore_free(rdev, &ib->semaphore, NULL);
+ }
radeon_ring_ib_execute(rdev, ib->ring, ib);
r = radeon_fence_emit(rdev, &ib->fence, ib->ring);
if (r) {
diff --git a/drivers/gpu/drm/radeon/radeon_semaphore.c b/drivers/gpu/drm/radeon/radeon_semaphore.c
index e2ace5d..7cc78de 100644
--- a/drivers/gpu/drm/radeon/radeon_semaphore.c
+++ b/drivers/gpu/drm/radeon/radeon_semaphore.c
@@ -68,70 +68,49 @@ void radeon_semaphore_emit_wait(struct radeon_device *rdev, int ring,
radeon_semaphore_ring_emit(rdev, ring, &rdev->ring[ring], semaphore, true);
}
+/* caller must hold ring lock */
int radeon_semaphore_sync_rings(struct radeon_device *rdev,
struct radeon_semaphore *semaphore,
- bool sync_to[RADEON_NUM_RINGS],
- int dst_ring)
+ int signaler, int waiter)
{
- int i = 0, r;
+ int r;
- mutex_lock(&rdev->ring_lock);
- r = radeon_ring_alloc(rdev, &rdev->ring[dst_ring], RADEON_NUM_RINGS * 8);
- if (r) {
- goto error;
+ /* no need to signal and wait on the same ring */
+ if (signaler == waiter) {
+ return 0;
}
- for (i = 0; i < RADEON_NUM_RINGS; ++i) {
- /* no need to sync to our own or unused rings */
- if (!sync_to[i] || i == dst_ring)
- continue;
-
- /* prevent GPU deadlocks */
- if (!rdev->ring[i].ready) {
- dev_err(rdev->dev, "Trying to sync to a disabled ring!");
- r = -EINVAL;
- goto error;
- }
-
- r = radeon_ring_alloc(rdev, &rdev->ring[i], 8);
- if (r) {
- goto error;
- }
-
- radeon_semaphore_emit_signal(rdev, i, semaphore);
- radeon_semaphore_emit_wait(rdev, dst_ring, semaphore);
+ /* prevent GPU deadlocks */
+ if (!rdev->ring[signaler].ready) {
+ dev_err(rdev->dev, "Trying to sync to a disabled ring!");
+ return -EINVAL;
+ }
- radeon_ring_commit(rdev, &rdev->ring[i]);
+ r = radeon_ring_alloc(rdev, &rdev->ring[signaler], 8);
+ if (r) {
+ return r;
}
+ radeon_semaphore_emit_signal(rdev, signaler, semaphore);
+ radeon_ring_commit(rdev, &rdev->ring[signaler]);
- radeon_ring_commit(rdev, &rdev->ring[dst_ring]);
- mutex_unlock(&rdev->ring_lock);
+ /* we assume caller has already allocated space on waiters ring */
+ radeon_semaphore_emit_wait(rdev, waiter, semaphore);
return 0;
-
-error:
- /* unlock all locks taken so far */
- for (--i; i >= 0; --i) {
- if (sync_to[i] || i == dst_ring) {
- radeon_ring_undo(&rdev->ring[i]);
- }
- }
- radeon_ring_undo(&rdev->ring[dst_ring]);
- mutex_unlock(&rdev->ring_lock);
- return r;
}
void radeon_semaphore_free(struct radeon_device *rdev,
- struct radeon_semaphore *semaphore,
+ struct radeon_semaphore **semaphore,
struct radeon_fence *fence)
{
- if (semaphore == NULL) {
+ if (semaphore == NULL || *semaphore == NULL) {
return;
}
- if (semaphore->waiters > 0) {
+ if ((*semaphore)->waiters > 0) {
dev_err(rdev->dev, "semaphore %p has more waiters than signalers,"
- " hardware lockup imminent!\n", semaphore);
+ " hardware lockup imminent!\n", *semaphore);
}
- radeon_sa_bo_free(rdev, &semaphore->sa_bo, fence);
- kfree(semaphore);
+ radeon_sa_bo_free(rdev, &(*semaphore)->sa_bo, fence);
+ kfree(*semaphore);
+ *semaphore = NULL;
}
diff --git a/drivers/gpu/drm/radeon/radeon_test.c b/drivers/gpu/drm/radeon/radeon_test.c
index eace3a5..ff6ac4b 100644
--- a/drivers/gpu/drm/radeon/radeon_test.c
+++ b/drivers/gpu/drm/radeon/radeon_test.c
@@ -303,8 +303,7 @@ void radeon_test_ring_sync(struct radeon_device *rdev,
}
out_cleanup:
- if (semaphore)
- radeon_semaphore_free(rdev, semaphore, NULL);
+ radeon_semaphore_free(rdev, &semaphore, NULL);
if (fence1)
radeon_fence_unref(&fence1);
@@ -422,8 +421,7 @@ void radeon_test_ring_sync2(struct radeon_device *rdev,
}
out_cleanup:
- if (semaphore)
- radeon_semaphore_free(rdev, semaphore, NULL);
+ radeon_semaphore_free(rdev, &semaphore, NULL);
if (fenceA)
radeon_fence_unref(&fenceA);
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 860e730..a7f9007 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -223,7 +223,6 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
struct radeon_device *rdev;
uint64_t old_start, new_start;
struct radeon_fence *fence;
- struct radeon_semaphore *sem = NULL;
int r, ridx;
rdev = radeon_get_rdev(bo->bdev);
@@ -262,31 +261,12 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
/* sync other rings */
fence = bo->sync_obj;
- if (fence && fence->ring != ridx
- && !radeon_fence_signaled(fence)) {
- bool sync_to_ring[RADEON_NUM_RINGS] = { };
- sync_to_ring[fence->ring] = true;
-
- r = radeon_semaphore_create(rdev, &sem);
- if (r) {
- return r;
- }
-
- r = radeon_semaphore_sync_rings(rdev, sem, sync_to_ring, ridx);
- if (r) {
- radeon_semaphore_free(rdev, sem, NULL);
- return r;
- }
- }
-
- fence = NULL;
r = radeon_copy(rdev, old_start, new_start,
new_mem->num_pages * (PAGE_SIZE / RADEON_GPU_PAGE_SIZE), /* GPU pages */
&fence);
/* FIXME: handle copy error */
r = ttm_bo_move_accel_cleanup(bo, (void *)fence, NULL,
evict, no_wait_reserve, no_wait_gpu, new_mem);
- radeon_semaphore_free(rdev, sem, fence);
radeon_fence_unref(&fence);
return r;
}
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/5] drm/radeon: WIP remove cs_mutex
2012-05-11 10:10 RFC: Removal of some mutexes from the radeon driver Christian König
` (2 preceding siblings ...)
2012-05-11 10:10 ` [PATCH 3/5] drm/radeon: rework ring syncing code Christian König
@ 2012-05-11 10:10 ` Christian König
2012-05-11 10:10 ` [PATCH 5/5] drm/radeon: WIP remove vmram_mutex Christian König
2012-05-11 10:12 ` RFC: Removal of some mutexes from the radeon driver Dave Airlie
5 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2012-05-11 10:10 UTC (permalink / raw)
To: dri-devel
There are probably some missed corner cases, but at least try to remove or
replace it with a vm mutex where it is still needed.
Signed-off-by: Christian König <deathsimple@vodafone.de>
---
drivers/gpu/drm/radeon/radeon.h | 44 +-------------------------------
drivers/gpu/drm/radeon/radeon_cs.c | 7 ++---
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
drivers/gpu/drm/radeon/radeon_gart.c | 16 ++++++------
drivers/gpu/drm/radeon/radeon_gem.c | 2 --
5 files changed, 12 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index e00c50d..8769217 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -159,48 +159,6 @@ static inline int radeon_atrm_get_bios_chunk(uint8_t *bios, int offset, int len)
#endif
bool radeon_get_bios(struct radeon_device *rdev);
-
-/*
- * Mutex which allows recursive locking from the same process.
- */
-struct radeon_mutex {
- struct mutex mutex;
- struct task_struct *owner;
- int level;
-};
-
-static inline void radeon_mutex_init(struct radeon_mutex *mutex)
-{
- mutex_init(&mutex->mutex);
- mutex->owner = NULL;
- mutex->level = 0;
-}
-
-static inline void radeon_mutex_lock(struct radeon_mutex *mutex)
-{
- if (mutex_trylock(&mutex->mutex)) {
- /* The mutex was unlocked before, so it's ours now */
- mutex->owner = current;
- } else if (mutex->owner != current) {
- /* Another process locked the mutex, take it */
- mutex_lock(&mutex->mutex);
- mutex->owner = current;
- }
- /* Otherwise the mutex was already locked by this process */
-
- mutex->level++;
-}
-
-static inline void radeon_mutex_unlock(struct radeon_mutex *mutex)
-{
- if (--mutex->level > 0)
- return;
-
- mutex->owner = NULL;
- mutex_unlock(&mutex->mutex);
-}
-
-
/*
* Dummy page
*/
@@ -1530,7 +1488,6 @@ struct radeon_device {
struct radeon_gem gem;
struct radeon_pm pm;
uint32_t bios_scratch[RADEON_BIOS_NUM_SCRATCH];
- struct radeon_mutex cs_mutex;
struct radeon_wb wb;
struct radeon_dummy_page dummy_page;
bool shutdown;
@@ -1564,6 +1521,7 @@ struct radeon_device {
struct radeon_debugfs debugfs[RADEON_DEBUGFS_MAX_COMPONENTS];
unsigned debugfs_count;
/* virtual memory */
+ struct mutex vm_mutex;
struct radeon_vm_manager vm_manager;
};
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
index d295821..d1ead9c 100644
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -438,6 +438,7 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev,
}
mutex_lock(&vm->mutex);
+ mutex_lock(&rdev->vm_mutex);
r = radeon_vm_bind(rdev, vm);
if (r) {
goto out;
@@ -474,6 +475,7 @@ out:
}
vm->fence = radeon_fence_ref(parser->ib.fence);
}
+ mutex_unlock(&rdev->vm_mutex);
mutex_unlock(&fpriv->vm.mutex);
return r;
}
@@ -494,9 +496,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
struct radeon_cs_parser parser;
int r;
- radeon_mutex_lock(&rdev->cs_mutex);
if (!rdev->accel_working) {
- radeon_mutex_unlock(&rdev->cs_mutex);
return -EBUSY;
}
/* initialize parser */
@@ -510,7 +510,6 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
DRM_ERROR("Failed to initialize parser !\n");
radeon_cs_parser_fini(&parser, r);
r = radeon_cs_handle_lockup(rdev, r);
- radeon_mutex_unlock(&rdev->cs_mutex);
return r;
}
r = radeon_cs_parser_relocs(&parser);
@@ -519,7 +518,6 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
DRM_ERROR("Failed to parse relocation %d!\n", r);
radeon_cs_parser_fini(&parser, r);
r = radeon_cs_handle_lockup(rdev, r);
- radeon_mutex_unlock(&rdev->cs_mutex);
return r;
}
r = radeon_cs_ib_chunk(rdev, &parser);
@@ -533,7 +531,6 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
out:
radeon_cs_parser_fini(&parser, r);
r = radeon_cs_handle_lockup(rdev, r);
- radeon_mutex_unlock(&rdev->cs_mutex);
return r;
}
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index e1bc7e9..7ddab8b 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -723,7 +723,6 @@ int radeon_device_init(struct radeon_device *rdev,
/* mutex initialization are all done here so we
* can recall function without having locking issues */
- radeon_mutex_init(&rdev->cs_mutex);
mutex_init(&rdev->ring_lock);
mutex_init(&rdev->dc_hw_i2c_mutex);
if (rdev->family >= CHIP_R600)
@@ -735,6 +734,7 @@ int radeon_device_init(struct radeon_device *rdev,
init_waitqueue_head(&rdev->irq.vblank_queue);
init_waitqueue_head(&rdev->irq.idle_queue);
/* initialize vm here */
+ mutex_init(&rdev->vm_mutex);
rdev->vm_manager.use_bitmap = 1;
rdev->vm_manager.max_pfn = 1 << 20;
INIT_LIST_HEAD(&rdev->vm_manager.lru_vm);
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c
index 8e9ef34..8076794 100644
--- a/drivers/gpu/drm/radeon/radeon_gart.c
+++ b/drivers/gpu/drm/radeon/radeon_gart.c
@@ -356,13 +356,13 @@ int radeon_vm_manager_suspend(struct radeon_device *rdev)
{
struct radeon_vm *vm, *tmp;
- radeon_mutex_lock(&rdev->cs_mutex);
+ mutex_lock(&rdev->vm_mutex);
/* unbind all active vm */
list_for_each_entry_safe(vm, tmp, &rdev->vm_manager.lru_vm, list) {
radeon_vm_unbind_locked(rdev, vm);
}
rdev->vm_manager.funcs->fini(rdev);
- radeon_mutex_unlock(&rdev->cs_mutex);
+ mutex_unlock(&rdev->vm_mutex);
return radeon_sa_bo_manager_suspend(rdev, &rdev->vm_manager.sa_manager);
}
@@ -478,9 +478,9 @@ int radeon_vm_bo_add(struct radeon_device *rdev,
if (last_pfn > vm->last_pfn) {
/* grow va space 32M by 32M */
unsigned align = ((32 << 20) >> 12) - 1;
- radeon_mutex_lock(&rdev->cs_mutex);
+ mutex_lock(&rdev->vm_mutex);
radeon_vm_unbind_locked(rdev, vm);
- radeon_mutex_unlock(&rdev->cs_mutex);
+ mutex_unlock(&rdev->vm_mutex);
vm->last_pfn = (last_pfn + align) & ~align;
}
head = &vm->va;
@@ -596,9 +596,9 @@ int radeon_vm_bo_rmv(struct radeon_device *rdev,
return 0;
mutex_lock(&vm->mutex);
- radeon_mutex_lock(&rdev->cs_mutex);
+ mutex_lock(&rdev->vm_mutex);
radeon_vm_bo_update_pte(rdev, vm, bo, NULL);
- radeon_mutex_unlock(&rdev->cs_mutex);
+ mutex_unlock(&rdev->vm_mutex);
list_del(&bo_va->vm_list);
mutex_unlock(&vm->mutex);
list_del(&bo_va->bo_list);
@@ -643,9 +643,9 @@ void radeon_vm_fini(struct radeon_device *rdev, struct radeon_vm *vm)
mutex_lock(&vm->mutex);
- radeon_mutex_lock(&rdev->cs_mutex);
+ mutex_lock(&rdev->vm_mutex);
radeon_vm_unbind_locked(rdev, vm);
- radeon_mutex_unlock(&rdev->cs_mutex);
+ mutex_unlock(&rdev->vm_mutex);
/* remove all bo */
r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false);
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
index e15cb1f..bc5ad73 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -157,11 +157,9 @@ void radeon_gem_object_close(struct drm_gem_object *obj,
static int radeon_gem_handle_lockup(struct radeon_device *rdev, int r)
{
if (r == -EDEADLK) {
- radeon_mutex_lock(&rdev->cs_mutex);
r = radeon_gpu_reset(rdev);
if (!r)
r = -EAGAIN;
- radeon_mutex_unlock(&rdev->cs_mutex);
}
return r;
}
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/5] drm/radeon: WIP remove vmram_mutex
2012-05-11 10:10 RFC: Removal of some mutexes from the radeon driver Christian König
` (3 preceding siblings ...)
2012-05-11 10:10 ` [PATCH 4/5] drm/radeon: WIP remove cs_mutex Christian König
@ 2012-05-11 10:10 ` Christian König
2012-05-11 14:41 ` Jerome Glisse
2012-05-11 10:12 ` RFC: Removal of some mutexes from the radeon driver Dave Airlie
5 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2012-05-11 10:10 UTC (permalink / raw)
To: dri-devel
Even more heretic than the last one. The mutex is
probably good for something, I just can't see what
that is at the moment.
Signed-off-by: Christian König <deathsimple@vodafone.de>
---
drivers/gpu/drm/radeon/radeon.h | 1 -
drivers/gpu/drm/radeon/radeon_device.c | 1 -
drivers/gpu/drm/radeon/radeon_object.c | 4 ----
drivers/gpu/drm/radeon/radeon_pm.c | 2 --
drivers/gpu/drm/radeon/radeon_ttm.c | 26 --------------------------
5 files changed, 34 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 8769217..c2753e7 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -1509,7 +1509,6 @@ struct radeon_device {
struct work_struct audio_work;
int num_crtc; /* number of crtcs */
struct mutex dc_hw_i2c_mutex; /* display controller hw i2c mutex */
- struct mutex vram_mutex;
struct r600_audio audio; /* audio stuff */
struct notifier_block acpi_nb;
/* only one userspace can use Hyperz features or CMASK at a time */
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 7ddab8b..24e185c 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -729,7 +729,6 @@ int radeon_device_init(struct radeon_device *rdev,
spin_lock_init(&rdev->ih.lock);
mutex_init(&rdev->gem.mutex);
mutex_init(&rdev->pm.mutex);
- mutex_init(&rdev->vram_mutex);
INIT_LIST_HEAD(&rdev->gem.objects);
init_waitqueue_head(&rdev->irq.vblank_queue);
init_waitqueue_head(&rdev->irq.idle_queue);
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index df6a4db..5fa2b1b 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -152,11 +152,9 @@ retry:
INIT_LIST_HEAD(&bo->va);
radeon_ttm_placement_from_domain(bo, domain);
/* Kernel allocation are uninterruptible */
- mutex_lock(&rdev->vram_mutex);
r = ttm_bo_init(&rdev->mman.bdev, &bo->tbo, size, type,
&bo->placement, page_align, 0, !kernel, NULL,
acc_size, &radeon_ttm_bo_destroy);
- mutex_unlock(&rdev->vram_mutex);
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS) {
if (domain == RADEON_GEM_DOMAIN_VRAM) {
@@ -217,9 +215,7 @@ void radeon_bo_unref(struct radeon_bo **bo)
return;
rdev = (*bo)->rdev;
tbo = &((*bo)->tbo);
- mutex_lock(&rdev->vram_mutex);
ttm_bo_unref(&tbo);
- mutex_unlock(&rdev->vram_mutex);
if (tbo == NULL)
*bo = NULL;
}
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 0882554..e8fba26 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -251,7 +251,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
return;
mutex_lock(&rdev->ddev->struct_mutex);
- mutex_lock(&rdev->vram_mutex);
mutex_lock(&rdev->ring_lock);
/* gui idle int has issues on older chips it seems */
@@ -303,7 +302,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
mutex_unlock(&rdev->ring_lock);
- mutex_unlock(&rdev->vram_mutex);
mutex_unlock(&rdev->ddev->struct_mutex);
}
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index a7f9007..c0a8647 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -771,26 +771,6 @@ void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size)
man->size = size >> PAGE_SHIFT;
}
-static struct vm_operations_struct radeon_ttm_vm_ops;
-static const struct vm_operations_struct *ttm_vm_ops = NULL;
-
-static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
-{
- struct ttm_buffer_object *bo;
- struct radeon_device *rdev;
- int r;
-
- bo = (struct ttm_buffer_object *)vma->vm_private_data;
- if (bo == NULL) {
- return VM_FAULT_NOPAGE;
- }
- rdev = radeon_get_rdev(bo->bdev);
- mutex_lock(&rdev->vram_mutex);
- r = ttm_vm_ops->fault(vma, vmf);
- mutex_unlock(&rdev->vram_mutex);
- return r;
-}
-
int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
{
struct drm_file *file_priv;
@@ -810,12 +790,6 @@ int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
if (unlikely(r != 0)) {
return r;
}
- if (unlikely(ttm_vm_ops == NULL)) {
- ttm_vm_ops = vma->vm_ops;
- radeon_ttm_vm_ops = *ttm_vm_ops;
- radeon_ttm_vm_ops.fault = &radeon_ttm_fault;
- }
- vma->vm_ops = &radeon_ttm_vm_ops;
return 0;
}
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: RFC: Removal of some mutexes from the radeon driver
2012-05-11 10:10 RFC: Removal of some mutexes from the radeon driver Christian König
` (4 preceding siblings ...)
2012-05-11 10:10 ` [PATCH 5/5] drm/radeon: WIP remove vmram_mutex Christian König
@ 2012-05-11 10:12 ` Dave Airlie
2012-05-11 11:54 ` Christian König
5 siblings, 1 reply; 12+ messages in thread
From: Dave Airlie @ 2012-05-11 10:12 UTC (permalink / raw)
To: Christian König; +Cc: dri-devel
On Fri, May 11, 2012 at 11:10 AM, Christian König
<deathsimple@vodafone.de> wrote:
> Hi everybody,
>
> well the following patches remove the cs and vram mutex from the radeon driver
> and so are something very experimental. The first three just move what I know
> to be still critical into the protection of the ring mutex, and the other two
> actually remove the mutexs.
>
> Interestingly it still survives a couple of hours running the following script:
>
> while true
> do
> for (( x = 10, y = 45, i = 0; $i < 144; i = $i + 1 ))
> do
> glxgears -geometry 100x100+$x+$y > /dev/null 2>&1 &
> x=$(expr $x + 120)
> if [ $x -gt 1920 ]
> then
> x=10
> y=$(expr $y + 110)
> fi
> done
>
> sleep 30
>
> killall glxgears
> done
>
> So the mutexes doesn't seem to protected something so critical and I'm
> wondering why we still have them so widely locked.
>
> Cheers,
> Christian.
>
> PS: I'm away for the next week or so don't expect any response soon.
The vram mutex is to block access to the VRAM during reclocking.
So if you reclocks a lot while running stuff you might notice.
Dave.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: RFC: Removal of some mutexes from the radeon driver
2012-05-11 10:12 ` RFC: Removal of some mutexes from the radeon driver Dave Airlie
@ 2012-05-11 11:54 ` Christian König
2012-05-14 15:14 ` Jerome Glisse
0 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2012-05-11 11:54 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On 11.05.2012 12:12, Dave Airlie wrote:
> On Fri, May 11, 2012 at 11:10 AM, Christian König
> <deathsimple@vodafone.de> wrote:
>> Hi everybody,
>>
>> well the following patches remove the cs and vram mutex from the radeon driver
>> and so are something very experimental. The first three just move what I know
>> to be still critical into the protection of the ring mutex, and the other two
>> actually remove the mutexs.
>>
>> Interestingly it still survives a couple of hours running the following script:
>>
>> while true
>> do
>> for (( x = 10, y = 45, i = 0; $i< 144; i = $i + 1 ))
>> do
>> glxgears -geometry 100x100+$x+$y> /dev/null 2>&1&
>> x=$(expr $x + 120)
>> if [ $x -gt 1920 ]
>> then
>> x=10
>> y=$(expr $y + 110)
>> fi
>> done
>>
>> sleep 30
>>
>> killall glxgears
>> done
>>
>> So the mutexes doesn't seem to protected something so critical and I'm
>> wondering why we still have them so widely locked.
>>
>> Cheers,
>> Christian.
>>
>> PS: I'm away for the next week or so don't expect any response soon.
> The vram mutex is to block access to the VRAM during reclocking.
>
> So if you reclocks a lot while running stuff you might notice.
Ah, thx for that info it suddenly starts to make sense. Also not using
an APU for testing might also help triggering the problem, but in turn
that means we can avoid taking that lock on APUs.
Cheers,
Christian.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] drm/radeon: WIP remove vmram_mutex
2012-05-11 10:10 ` [PATCH 5/5] drm/radeon: WIP remove vmram_mutex Christian König
@ 2012-05-11 14:41 ` Jerome Glisse
2012-05-11 14:44 ` Jerome Glisse
0 siblings, 1 reply; 12+ messages in thread
From: Jerome Glisse @ 2012-05-11 14:41 UTC (permalink / raw)
To: Christian König; +Cc: dri-devel
On Fri, May 11, 2012 at 6:10 AM, Christian König
<deathsimple@vodafone.de> wrote:
> Even more heretic than the last one. The mutex is
> probably good for something, I just can't see what
> that is at the moment.
>
> Signed-off-by: Christian König <deathsimple@vodafone.de>
> ---
> drivers/gpu/drm/radeon/radeon.h | 1 -
> drivers/gpu/drm/radeon/radeon_device.c | 1 -
> drivers/gpu/drm/radeon/radeon_object.c | 4 ----
> drivers/gpu/drm/radeon/radeon_pm.c | 2 --
> drivers/gpu/drm/radeon/radeon_ttm.c | 26 --------------------------
> 5 files changed, 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 8769217..c2753e7 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -1509,7 +1509,6 @@ struct radeon_device {
> struct work_struct audio_work;
> int num_crtc; /* number of crtcs */
> struct mutex dc_hw_i2c_mutex; /* display controller hw i2c mutex */
> - struct mutex vram_mutex;
> struct r600_audio audio; /* audio stuff */
> struct notifier_block acpi_nb;
> /* only one userspace can use Hyperz features or CMASK at a time */
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index 7ddab8b..24e185c 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -729,7 +729,6 @@ int radeon_device_init(struct radeon_device *rdev,
> spin_lock_init(&rdev->ih.lock);
> mutex_init(&rdev->gem.mutex);
> mutex_init(&rdev->pm.mutex);
> - mutex_init(&rdev->vram_mutex);
> INIT_LIST_HEAD(&rdev->gem.objects);
> init_waitqueue_head(&rdev->irq.vblank_queue);
> init_waitqueue_head(&rdev->irq.idle_queue);
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index df6a4db..5fa2b1b 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -152,11 +152,9 @@ retry:
> INIT_LIST_HEAD(&bo->va);
> radeon_ttm_placement_from_domain(bo, domain);
> /* Kernel allocation are uninterruptible */
> - mutex_lock(&rdev->vram_mutex);
> r = ttm_bo_init(&rdev->mman.bdev, &bo->tbo, size, type,
> &bo->placement, page_align, 0, !kernel, NULL,
> acc_size, &radeon_ttm_bo_destroy);
> - mutex_unlock(&rdev->vram_mutex);
> if (unlikely(r != 0)) {
> if (r != -ERESTARTSYS) {
> if (domain == RADEON_GEM_DOMAIN_VRAM) {
> @@ -217,9 +215,7 @@ void radeon_bo_unref(struct radeon_bo **bo)
> return;
> rdev = (*bo)->rdev;
> tbo = &((*bo)->tbo);
> - mutex_lock(&rdev->vram_mutex);
> ttm_bo_unref(&tbo);
> - mutex_unlock(&rdev->vram_mutex);
> if (tbo == NULL)
> *bo = NULL;
> }
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index 0882554..e8fba26 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -251,7 +251,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
> return;
>
> mutex_lock(&rdev->ddev->struct_mutex);
> - mutex_lock(&rdev->vram_mutex);
> mutex_lock(&rdev->ring_lock);
>
> /* gui idle int has issues on older chips it seems */
> @@ -303,7 +302,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
> rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
>
> mutex_unlock(&rdev->ring_lock);
> - mutex_unlock(&rdev->vram_mutex);
> mutex_unlock(&rdev->ddev->struct_mutex);
> }
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index a7f9007..c0a8647 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -771,26 +771,6 @@ void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size)
> man->size = size >> PAGE_SHIFT;
> }
>
> -static struct vm_operations_struct radeon_ttm_vm_ops;
> -static const struct vm_operations_struct *ttm_vm_ops = NULL;
> -
> -static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> -{
> - struct ttm_buffer_object *bo;
> - struct radeon_device *rdev;
> - int r;
> -
> - bo = (struct ttm_buffer_object *)vma->vm_private_data;
> - if (bo == NULL) {
> - return VM_FAULT_NOPAGE;
> - }
> - rdev = radeon_get_rdev(bo->bdev);
> - mutex_lock(&rdev->vram_mutex);
> - r = ttm_vm_ops->fault(vma, vmf);
> - mutex_unlock(&rdev->vram_mutex);
> - return r;
> -}
> -
> int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> struct drm_file *file_priv;
> @@ -810,12 +790,6 @@ int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
> if (unlikely(r != 0)) {
> return r;
> }
> - if (unlikely(ttm_vm_ops == NULL)) {
> - ttm_vm_ops = vma->vm_ops;
> - radeon_ttm_vm_ops = *ttm_vm_ops;
> - radeon_ttm_vm_ops.fault = &radeon_ttm_fault;
> - }
> - vma->vm_ops = &radeon_ttm_vm_ops;
> return 0;
> }
>
Why are you removing the ttm fault stuff ? And does the driver keep
working without this ? I would be surprise.
Cheers,
Jerome
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] drm/radeon: WIP remove vmram_mutex
2012-05-11 14:41 ` Jerome Glisse
@ 2012-05-11 14:44 ` Jerome Glisse
2012-05-11 15:12 ` Christian König
0 siblings, 1 reply; 12+ messages in thread
From: Jerome Glisse @ 2012-05-11 14:44 UTC (permalink / raw)
To: Christian König; +Cc: dri-devel
On Fri, May 11, 2012 at 10:41 AM, Jerome Glisse <j.glisse@gmail.com> wrote:
> On Fri, May 11, 2012 at 6:10 AM, Christian König
> <deathsimple@vodafone.de> wrote:
>> Even more heretic than the last one. The mutex is
>> probably good for something, I just can't see what
>> that is at the moment.
>>
>> Signed-off-by: Christian König <deathsimple@vodafone.de>
>> ---
>> drivers/gpu/drm/radeon/radeon.h | 1 -
>> drivers/gpu/drm/radeon/radeon_device.c | 1 -
>> drivers/gpu/drm/radeon/radeon_object.c | 4 ----
>> drivers/gpu/drm/radeon/radeon_pm.c | 2 --
>> drivers/gpu/drm/radeon/radeon_ttm.c | 26 --------------------------
>> 5 files changed, 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
>> index 8769217..c2753e7 100644
>> --- a/drivers/gpu/drm/radeon/radeon.h
>> +++ b/drivers/gpu/drm/radeon/radeon.h
>> @@ -1509,7 +1509,6 @@ struct radeon_device {
>> struct work_struct audio_work;
>> int num_crtc; /* number of crtcs */
>> struct mutex dc_hw_i2c_mutex; /* display controller hw i2c mutex */
>> - struct mutex vram_mutex;
>> struct r600_audio audio; /* audio stuff */
>> struct notifier_block acpi_nb;
>> /* only one userspace can use Hyperz features or CMASK at a time */
>> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
>> index 7ddab8b..24e185c 100644
>> --- a/drivers/gpu/drm/radeon/radeon_device.c
>> +++ b/drivers/gpu/drm/radeon/radeon_device.c
>> @@ -729,7 +729,6 @@ int radeon_device_init(struct radeon_device *rdev,
>> spin_lock_init(&rdev->ih.lock);
>> mutex_init(&rdev->gem.mutex);
>> mutex_init(&rdev->pm.mutex);
>> - mutex_init(&rdev->vram_mutex);
>> INIT_LIST_HEAD(&rdev->gem.objects);
>> init_waitqueue_head(&rdev->irq.vblank_queue);
>> init_waitqueue_head(&rdev->irq.idle_queue);
>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
>> index df6a4db..5fa2b1b 100644
>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>> @@ -152,11 +152,9 @@ retry:
>> INIT_LIST_HEAD(&bo->va);
>> radeon_ttm_placement_from_domain(bo, domain);
>> /* Kernel allocation are uninterruptible */
>> - mutex_lock(&rdev->vram_mutex);
>> r = ttm_bo_init(&rdev->mman.bdev, &bo->tbo, size, type,
>> &bo->placement, page_align, 0, !kernel, NULL,
>> acc_size, &radeon_ttm_bo_destroy);
>> - mutex_unlock(&rdev->vram_mutex);
>> if (unlikely(r != 0)) {
>> if (r != -ERESTARTSYS) {
>> if (domain == RADEON_GEM_DOMAIN_VRAM) {
>> @@ -217,9 +215,7 @@ void radeon_bo_unref(struct radeon_bo **bo)
>> return;
>> rdev = (*bo)->rdev;
>> tbo = &((*bo)->tbo);
>> - mutex_lock(&rdev->vram_mutex);
>> ttm_bo_unref(&tbo);
>> - mutex_unlock(&rdev->vram_mutex);
>> if (tbo == NULL)
>> *bo = NULL;
>> }
>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
>> index 0882554..e8fba26 100644
>> --- a/drivers/gpu/drm/radeon/radeon_pm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
>> @@ -251,7 +251,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
>> return;
>>
>> mutex_lock(&rdev->ddev->struct_mutex);
>> - mutex_lock(&rdev->vram_mutex);
>> mutex_lock(&rdev->ring_lock);
>>
>> /* gui idle int has issues on older chips it seems */
>> @@ -303,7 +302,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
>> rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
>>
>> mutex_unlock(&rdev->ring_lock);
>> - mutex_unlock(&rdev->vram_mutex);
>> mutex_unlock(&rdev->ddev->struct_mutex);
>> }
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index a7f9007..c0a8647 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -771,26 +771,6 @@ void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size)
>> man->size = size >> PAGE_SHIFT;
>> }
>>
>> -static struct vm_operations_struct radeon_ttm_vm_ops;
>> -static const struct vm_operations_struct *ttm_vm_ops = NULL;
>> -
>> -static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> -{
>> - struct ttm_buffer_object *bo;
>> - struct radeon_device *rdev;
>> - int r;
>> -
>> - bo = (struct ttm_buffer_object *)vma->vm_private_data;
>> - if (bo == NULL) {
>> - return VM_FAULT_NOPAGE;
>> - }
>> - rdev = radeon_get_rdev(bo->bdev);
>> - mutex_lock(&rdev->vram_mutex);
>> - r = ttm_vm_ops->fault(vma, vmf);
>> - mutex_unlock(&rdev->vram_mutex);
>> - return r;
>> -}
>> -
>> int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
>> {
>> struct drm_file *file_priv;
>> @@ -810,12 +790,6 @@ int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
>> if (unlikely(r != 0)) {
>> return r;
>> }
>> - if (unlikely(ttm_vm_ops == NULL)) {
>> - ttm_vm_ops = vma->vm_ops;
>> - radeon_ttm_vm_ops = *ttm_vm_ops;
>> - radeon_ttm_vm_ops.fault = &radeon_ttm_fault;
>> - }
>> - vma->vm_ops = &radeon_ttm_vm_ops;
>> return 0;
>> }
>>
>
> Why are you removing the ttm fault stuff ? And does the driver keep
> working without this ? I would be surprise.
>
> Cheers,
> Jerome
Oh i forgot ttm already fill the vma with its own callback.
Cheers,
Jerome
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] drm/radeon: WIP remove vmram_mutex
2012-05-11 14:44 ` Jerome Glisse
@ 2012-05-11 15:12 ` Christian König
0 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2012-05-11 15:12 UTC (permalink / raw)
To: Jerome Glisse; +Cc: dri-devel
[-- Attachment #1: Type: text/plain, Size: 6587 bytes --]
On 11.05.2012 16:44, Jerome Glisse wrote:
> On Fri, May 11, 2012 at 10:41 AM, Jerome Glisse<j.glisse@gmail.com> wrote:
>> On Fri, May 11, 2012 at 6:10 AM, Christian König
>> <deathsimple@vodafone.de> wrote:
>>> Even more heretic than the last one. The mutex is
>>> probably good for something, I just can't see what
>>> that is at the moment.
>>>
>>> Signed-off-by: Christian König<deathsimple@vodafone.de>
>>> ---
>>> drivers/gpu/drm/radeon/radeon.h | 1 -
>>> drivers/gpu/drm/radeon/radeon_device.c | 1 -
>>> drivers/gpu/drm/radeon/radeon_object.c | 4 ----
>>> drivers/gpu/drm/radeon/radeon_pm.c | 2 --
>>> drivers/gpu/drm/radeon/radeon_ttm.c | 26 --------------------------
>>> 5 files changed, 34 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
>>> index 8769217..c2753e7 100644
>>> --- a/drivers/gpu/drm/radeon/radeon.h
>>> +++ b/drivers/gpu/drm/radeon/radeon.h
>>> @@ -1509,7 +1509,6 @@ struct radeon_device {
>>> struct work_struct audio_work;
>>> int num_crtc; /* number of crtcs */
>>> struct mutex dc_hw_i2c_mutex; /* display controller hw i2c mutex */
>>> - struct mutex vram_mutex;
>>> struct r600_audio audio; /* audio stuff */
>>> struct notifier_block acpi_nb;
>>> /* only one userspace can use Hyperz features or CMASK at a time */
>>> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
>>> index 7ddab8b..24e185c 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_device.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_device.c
>>> @@ -729,7 +729,6 @@ int radeon_device_init(struct radeon_device *rdev,
>>> spin_lock_init(&rdev->ih.lock);
>>> mutex_init(&rdev->gem.mutex);
>>> mutex_init(&rdev->pm.mutex);
>>> - mutex_init(&rdev->vram_mutex);
>>> INIT_LIST_HEAD(&rdev->gem.objects);
>>> init_waitqueue_head(&rdev->irq.vblank_queue);
>>> init_waitqueue_head(&rdev->irq.idle_queue);
>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
>>> index df6a4db..5fa2b1b 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>> @@ -152,11 +152,9 @@ retry:
>>> INIT_LIST_HEAD(&bo->va);
>>> radeon_ttm_placement_from_domain(bo, domain);
>>> /* Kernel allocation are uninterruptible */
>>> - mutex_lock(&rdev->vram_mutex);
>>> r = ttm_bo_init(&rdev->mman.bdev,&bo->tbo, size, type,
>>> &bo->placement, page_align, 0, !kernel, NULL,
>>> acc_size,&radeon_ttm_bo_destroy);
>>> - mutex_unlock(&rdev->vram_mutex);
>>> if (unlikely(r != 0)) {
>>> if (r != -ERESTARTSYS) {
>>> if (domain == RADEON_GEM_DOMAIN_VRAM) {
>>> @@ -217,9 +215,7 @@ void radeon_bo_unref(struct radeon_bo **bo)
>>> return;
>>> rdev = (*bo)->rdev;
>>> tbo =&((*bo)->tbo);
>>> - mutex_lock(&rdev->vram_mutex);
>>> ttm_bo_unref(&tbo);
>>> - mutex_unlock(&rdev->vram_mutex);
>>> if (tbo == NULL)
>>> *bo = NULL;
>>> }
>>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
>>> index 0882554..e8fba26 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_pm.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
>>> @@ -251,7 +251,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
>>> return;
>>>
>>> mutex_lock(&rdev->ddev->struct_mutex);
>>> - mutex_lock(&rdev->vram_mutex);
>>> mutex_lock(&rdev->ring_lock);
>>>
>>> /* gui idle int has issues on older chips it seems */
>>> @@ -303,7 +302,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
>>> rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
>>>
>>> mutex_unlock(&rdev->ring_lock);
>>> - mutex_unlock(&rdev->vram_mutex);
>>> mutex_unlock(&rdev->ddev->struct_mutex);
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>>> index a7f9007..c0a8647 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>> @@ -771,26 +771,6 @@ void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size)
>>> man->size = size>> PAGE_SHIFT;
>>> }
>>>
>>> -static struct vm_operations_struct radeon_ttm_vm_ops;
>>> -static const struct vm_operations_struct *ttm_vm_ops = NULL;
>>> -
>>> -static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>>> -{
>>> - struct ttm_buffer_object *bo;
>>> - struct radeon_device *rdev;
>>> - int r;
>>> -
>>> - bo = (struct ttm_buffer_object *)vma->vm_private_data;
>>> - if (bo == NULL) {
>>> - return VM_FAULT_NOPAGE;
>>> - }
>>> - rdev = radeon_get_rdev(bo->bdev);
>>> - mutex_lock(&rdev->vram_mutex);
>>> - r = ttm_vm_ops->fault(vma, vmf);
>>> - mutex_unlock(&rdev->vram_mutex);
>>> - return r;
>>> -}
>>> -
>>> int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
>>> {
>>> struct drm_file *file_priv;
>>> @@ -810,12 +790,6 @@ int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
>>> if (unlikely(r != 0)) {
>>> return r;
>>> }
>>> - if (unlikely(ttm_vm_ops == NULL)) {
>>> - ttm_vm_ops = vma->vm_ops;
>>> - radeon_ttm_vm_ops = *ttm_vm_ops;
>>> - radeon_ttm_vm_ops.fault =&radeon_ttm_fault;
>>> - }
>>> - vma->vm_ops =&radeon_ttm_vm_ops;
>>> return 0;
>>> }
>>>
>> Why are you removing the ttm fault stuff ? And does the driver keep
>> working without this ? I would be surprise.
>>
>> Cheers,
>> Jerome
> Oh i forgot ttm already fill the vma with its own callback.
>
> Cheers,
> Jerome
>
Removed it to just figure out why it is there in the first place, Dave
already explained it as a protection for accessing VRAM while we change
the memory clock. Currently just trying to look into all the parts of
the driver I still doesn't understand completely.
Anyway, I would suggest to change it to a rw_semaphore and read lock it
in most cases, and just write lock it while changing the memory clock,
see the attached patch for this.
Cheers,
Christian.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-drm-radeon-replace-vmram_mutex-with-mclk_lock.patch --]
[-- Type: text/x-patch; name="0001-drm-radeon-replace-vmram_mutex-with-mclk_lock.patch", Size: 5105 bytes --]
>From 2f8068d07e8dec198b61f096f5300a477a67a5b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <deathsimple@vodafone.de>
Date: Fri, 11 May 2012 14:57:18 +0200
Subject: [PATCH 1/2] drm/radeon: replace vmram_mutex with mclk_lock
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
It is a rw_semaphore now and only write locked
while changing the clock. Also the lock is renamed
to better reflect what it is protecting.
Signed-off-by: Christian König <deathsimple@vodafone.de>
---
drivers/gpu/drm/radeon/radeon.h | 3 ++-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
drivers/gpu/drm/radeon/radeon_object.c | 8 ++++----
drivers/gpu/drm/radeon/radeon_pm.c | 4 ++--
drivers/gpu/drm/radeon/radeon_ttm.c | 6 +++---
5 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index e00c50d..2417327 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -1055,6 +1055,8 @@ struct radeon_power_state {
struct radeon_pm {
struct mutex mutex;
+ /* write locked while reprogramming mclk */
+ struct rw_semaphore mclk_lock;
u32 active_crtcs;
int active_crtc_count;
int req_vblank;
@@ -1552,7 +1554,6 @@ struct radeon_device {
struct work_struct audio_work;
int num_crtc; /* number of crtcs */
struct mutex dc_hw_i2c_mutex; /* display controller hw i2c mutex */
- struct mutex vram_mutex;
struct r600_audio audio; /* audio stuff */
struct notifier_block acpi_nb;
/* only one userspace can use Hyperz features or CMASK at a time */
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index e1bc7e9..c74f770 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -730,7 +730,7 @@ int radeon_device_init(struct radeon_device *rdev,
spin_lock_init(&rdev->ih.lock);
mutex_init(&rdev->gem.mutex);
mutex_init(&rdev->pm.mutex);
- mutex_init(&rdev->vram_mutex);
+ init_rwsem(&rdev->pm.mclk_lock);
INIT_LIST_HEAD(&rdev->gem.objects);
init_waitqueue_head(&rdev->irq.vblank_queue);
init_waitqueue_head(&rdev->irq.idle_queue);
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index df6a4db..bc814e7 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -152,11 +152,11 @@ retry:
INIT_LIST_HEAD(&bo->va);
radeon_ttm_placement_from_domain(bo, domain);
/* Kernel allocation are uninterruptible */
- mutex_lock(&rdev->vram_mutex);
+ down_read(&rdev->pm.mclk_lock);
r = ttm_bo_init(&rdev->mman.bdev, &bo->tbo, size, type,
&bo->placement, page_align, 0, !kernel, NULL,
acc_size, &radeon_ttm_bo_destroy);
- mutex_unlock(&rdev->vram_mutex);
+ up_read(&rdev->pm.mclk_lock);
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS) {
if (domain == RADEON_GEM_DOMAIN_VRAM) {
@@ -217,9 +217,9 @@ void radeon_bo_unref(struct radeon_bo **bo)
return;
rdev = (*bo)->rdev;
tbo = &((*bo)->tbo);
- mutex_lock(&rdev->vram_mutex);
+ down_read(&rdev->pm.mclk_lock);
ttm_bo_unref(&tbo);
- mutex_unlock(&rdev->vram_mutex);
+ up_read(&rdev->pm.mclk_lock);
if (tbo == NULL)
*bo = NULL;
}
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 0882554..d13b6ae 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -251,7 +251,7 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
return;
mutex_lock(&rdev->ddev->struct_mutex);
- mutex_lock(&rdev->vram_mutex);
+ down_write(&rdev->pm.mclk_lock);
mutex_lock(&rdev->ring_lock);
/* gui idle int has issues on older chips it seems */
@@ -303,7 +303,7 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
mutex_unlock(&rdev->ring_lock);
- mutex_unlock(&rdev->vram_mutex);
+ up_write(&rdev->pm.mclk_lock);
mutex_unlock(&rdev->ddev->struct_mutex);
}
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index a7f9007..fc05ae3 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -785,9 +785,9 @@ static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
return VM_FAULT_NOPAGE;
}
rdev = radeon_get_rdev(bo->bdev);
- mutex_lock(&rdev->vram_mutex);
+ down_read(&rdev->pm.mclk_lock);
r = ttm_vm_ops->fault(vma, vmf);
- mutex_unlock(&rdev->vram_mutex);
+ up_read(&rdev->pm.mclk_lock);
return r;
}
@@ -810,7 +810,7 @@ int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
if (unlikely(r != 0)) {
return r;
}
- if (unlikely(ttm_vm_ops == NULL)) {
+ if (unlikely(ttm_vm_ops == NULL && !(rdev->flags & RADEON_IS_IGP))) {
ttm_vm_ops = vma->vm_ops;
radeon_ttm_vm_ops = *ttm_vm_ops;
radeon_ttm_vm_ops.fault = &radeon_ttm_fault;
--
1.7.9.5
[-- Attachment #3: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: RFC: Removal of some mutexes from the radeon driver
2012-05-11 11:54 ` Christian König
@ 2012-05-14 15:14 ` Jerome Glisse
0 siblings, 0 replies; 12+ messages in thread
From: Jerome Glisse @ 2012-05-14 15:14 UTC (permalink / raw)
To: Christian König; +Cc: dri-devel
On Fri, May 11, 2012 at 7:54 AM, Christian König
<deathsimple@vodafone.de> wrote:
> On 11.05.2012 12:12, Dave Airlie wrote:
>>
>> On Fri, May 11, 2012 at 11:10 AM, Christian König
>> <deathsimple@vodafone.de> wrote:
>>>
>>> Hi everybody,
>>>
>>> well the following patches remove the cs and vram mutex from the radeon
>>> driver
>>> and so are something very experimental. The first three just move what I
>>> know
>>> to be still critical into the protection of the ring mutex, and the other
>>> two
>>> actually remove the mutexs.
>>>
>>> Interestingly it still survives a couple of hours running the following
>>> script:
>>>
>>> while true
>>> do
>>> for (( x = 10, y = 45, i = 0; $i< 144; i = $i + 1 ))
>>> do
>>> glxgears -geometry 100x100+$x+$y> /dev/null 2>&1&
>>> x=$(expr $x + 120)
>>> if [ $x -gt 1920 ]
>>> then
>>> x=10
>>> y=$(expr $y + 110)
>>> fi
>>> done
>>>
>>> sleep 30
>>>
>>> killall glxgears
>>> done
>>>
>>> So the mutexes doesn't seem to protected something so critical and I'm
>>> wondering why we still have them so widely locked.
>>>
>>> Cheers,
>>> Christian.
>>>
>>> PS: I'm away for the next week or so don't expect any response soon.
>>
>> The vram mutex is to block access to the VRAM during reclocking.
>>
>> So if you reclocks a lot while running stuff you might notice.
>
>
> Ah, thx for that info it suddenly starts to make sense. Also not using an
> APU for testing might also help triggering the problem, but in turn that
> means we can avoid taking that lock on APUs.
>
> Cheers,
> Christian.
>
The first 3 looks good, i need to test and read more carefully the cs
-> vm mutex one but it looks good too.
Cheers,
Jerome
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-05-14 15:14 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-11 10:10 RFC: Removal of some mutexes from the radeon driver Christian König
2012-05-11 10:10 ` [PATCH 1/5] drm/radeon: remove radeon_fence_create Christian König
2012-05-11 10:10 ` [PATCH 2/5] drm/radeon: add infrastructure for advanced ring synchronization Christian König
2012-05-11 10:10 ` [PATCH 3/5] drm/radeon: rework ring syncing code Christian König
2012-05-11 10:10 ` [PATCH 4/5] drm/radeon: WIP remove cs_mutex Christian König
2012-05-11 10:10 ` [PATCH 5/5] drm/radeon: WIP remove vmram_mutex Christian König
2012-05-11 14:41 ` Jerome Glisse
2012-05-11 14:44 ` Jerome Glisse
2012-05-11 15:12 ` Christian König
2012-05-11 10:12 ` RFC: Removal of some mutexes from the radeon driver Dave Airlie
2012-05-11 11:54 ` Christian König
2012-05-14 15:14 ` Jerome Glisse
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.