On Fri, Jul 13, 2012 at 04:08:14PM +0200, Christian König wrote:

Otherwise we can encounter out of memory situations under extreme load.

Signed-off-by: Christian König deathsimple@vodafone.de

drivers/gpu/drm/radeon/radeon.h | 2 +- drivers/gpu/drm/radeon/radeon_sa.c | 72 +++++++++++++++++++++++++----------- 2 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 6715e4c..2cb355b 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -362,7 +362,7 @@ struct radeon_bo_list {

• alignment).

*/ struct radeon_sa_manager {

• spinlock_t lock;

• wait_queue_head_t wq; struct radeon_bo *bo; struct list_head *hole; struct list_head flist[RADEON_NUM_RINGS];

diff --git a/drivers/gpu/drm/radeon/radeon_sa.c b/drivers/gpu/drm/radeon/radeon_sa.c index 81dbb5b..b535fc4 100644 --- a/drivers/gpu/drm/radeon/radeon_sa.c +++ b/drivers/gpu/drm/radeon/radeon_sa.c @@ -54,7 +54,7 @@ int radeon_sa_bo_manager_init(struct radeon_device *rdev, { int i, r;

• spin_lock_init(&sa_manager->lock);

• init_waitqueue_head(&sa_manager->wq); sa_manager->bo = NULL; sa_manager->size = size; sa_manager->domain = domain;

@@ -211,6 +211,29 @@ static bool radeon_sa_bo_try_alloc(struct radeon_sa_manager *sa_manager, return false; }

+static bool radeon_sa_event(struct radeon_sa_manager *sa_manager,

• 	    unsigned size, unsigned align)
  

+{

• unsigned soffset, eoffset, wasted;
• int i;
• for (i = 0; i < RADEON_NUM_RINGS; ++i) {

• if (!list_empty(&sa_manager->flist[i])) {
  

• 	return true;
  

• }
  

• }
• soffset = radeon_sa_bo_hole_soffset(sa_manager);
• eoffset = radeon_sa_bo_hole_eoffset(sa_manager);
• wasted = (align - (soffset % align)) % align;
• if ((eoffset - soffset) >= (size + wasted)) {

• return true;
  

• }
• return false;

+}

This new function should come with a comment, per the new documentation rules.

static bool radeon_sa_bo_next_hole(struct radeon_sa_manager *sa_manager, struct radeon_fence **fences, unsigned *tries) @@ -297,8 +320,8 @@ int radeon_sa_bo_new(struct radeon_device *rdev, INIT_LIST_HEAD(&(*sa_bo)->olist); INIT_LIST_HEAD(&(*sa_bo)->flist);

• spin_lock(&sa_manager->lock);
• do {

• spin_lock(&sa_manager->wq.lock);
• while(1) { for (i = 0; i < RADEON_NUM_RINGS; ++i) { fences[i] = NULL; tries[i] = 0;

@@ -309,30 +332,34 @@ int radeon_sa_bo_new(struct radeon_device *rdev,

	if (radeon_sa_bo_try_alloc(sa_manager, *sa_bo,
				   size, align)) {

• 		spin_unlock(&sa_manager->lock);
  

• 		spin_unlock(&sa_manager->wq.lock);
  	return 0;
  }
  
  /* see if we can skip over some allocations */
  

  } while (radeon_sa_bo_next_hole(sa_manager, fences, tries));

• if (block) {
  

• 	spin_unlock(&sa_manager->lock);
  

• 	r = radeon_fence_wait_any(rdev, fences, false);
  

• 	spin_lock(&sa_manager->lock);
  

• 	if (r) {
  

• 		/* if we have nothing to wait for we
  

• 		   are practically out of memory */
  

• 		if (r == -ENOENT) {
  

• 			r = -ENOMEM;
  

• 		}
  

• 		goto out_err;
  

• 	}
  

• if (!block) {
  

• 	break;
  

• }
  

• spin_unlock(&sa_manager->wq.lock);
  

• r = radeon_fence_wait_any(rdev, fences, false);
  

• spin_lock(&sa_manager->wq.lock);
  

• /* if we have nothing to wait for block */
  

• if (r == -ENOENT) {
  

• 	r = wait_event_interruptible_locked(
  

• 		sa_manager->wq, 
  

• 		radeon_sa_event(sa_manager, size, align)
  

• 	);
  

• }
  

• if (r) {
  

• 	goto out_err;
  

  }

• } while (block);

• };

out_err:

• spin_unlock(&sa_manager->lock);

• spin_unlock(&sa_manager->wq.lock); kfree(*sa_bo); *sa_bo = NULL; return r;

@@ -348,7 +375,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);

• spin_lock(&sa_manager->wq.lock); if (fence && !radeon_fence_signaled(fence)) { (*sa_bo)->fence = radeon_fence_ref(fence); list_add_tail(&(*sa_bo)->flist,

@@ -356,7 +383,8 @@ void radeon_sa_bo_free(struct radeon_device *rdev, struct radeon_sa_bo **sa_bo, } else { radeon_sa_bo_remove_locked(*sa_bo); }

• spin_unlock(&sa_manager->lock);

• wake_up_all_locked(&sa_manager->wq);
• spin_unlock(&sa_manager->wq.lock); *sa_bo = NULL;

}

@@ -366,7 +394,7 @@ void radeon_sa_bo_dump_debug_info(struct radeon_sa_manager *sa_manager, { struct radeon_sa_bo *i;

• spin_lock(&sa_manager->lock);

• spin_lock(&sa_manager->wq.lock); list_for_each_entry(i, &sa_manager->olist, olist) { if (&i->olist == sa_manager->hole) { seq_printf(m, ">");

@@ -381,6 +409,6 @@ void radeon_sa_bo_dump_debug_info(struct radeon_sa_manager *sa_manager, } seq_printf(m, "\n"); }

• spin_unlock(&sa_manager->lock);

• spin_unlock(&sa_manager->wq.lock);

}

#endif

1.7.9.5

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian König wrote:

Const IBs are executed on the CE not the CP, so we can't fence them in the normal way.

So submit them directly before the IB instead, just as the documentation says.

Signed-off-by: Christian König deathsimple@vodafone.de

drivers/gpu/drm/radeon/r100.c | 2 +- drivers/gpu/drm/radeon/r600.c | 2 +- drivers/gpu/drm/radeon/radeon.h | 3 ++- drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index e0f5ae8..4ee5a74 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[6] = PACKET2(0); ib.ptr[7] = PACKET2(0); ib.length_dw = 8;

• r = radeon_ib_schedule(rdev, &ib);

• r = radeon_ib_schedule(rdev, &ib, NULL); if (r) { radeon_scratch_free(rdev, scratch); radeon_ib_free(rdev, &ib);

diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 3156d25..c2e5069 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); ib.ptr[2] = 0xDEADBEEF; ib.length_dw = 3;

• r = radeon_ib_schedule(rdev, &ib);

• r = radeon_ib_schedule(rdev, &ib, NULL); if (r) { radeon_scratch_free(rdev, scratch); radeon_ib_free(rdev, &ib);

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 2cb355b..2d7f06c 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -751,7 +751,8 @@ struct si_rlc { int radeon_ib_get(struct radeon_device *rdev, int ring, struct radeon_ib *ib, unsigned size); void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib); +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,

•        struct radeon_ib *const_ib);
  

int radeon_ib_pool_init(struct radeon_device *rdev); void radeon_ib_pool_fini(struct radeon_device *rdev); int radeon_ib_ring_tests(struct radeon_device *rdev); diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 553da67..d0be5d5 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser); parser->ib.vm_id = 0;

• r = radeon_ib_schedule(rdev, &parser->ib);

• r = radeon_ib_schedule(rdev, &parser->ib, NULL); if (r) { DRM_ERROR("Failed to schedule IB !\n"); }

@@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser);

• parser->ib.vm_id = vm->id;
• /* ib pool is bind at 0 in virtual address space,

• * so gpu_addr is the offset inside the pool bo
  

• */
  

• parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
• if ((rdev->family >= CHIP_TAHITI) && (parser->chunk_const_ib_idx != -1)) { parser->const_ib.vm_id = vm->id;

• /* ib pool is bind at 0 in virtual address space to gpu_addr is the
  

•  * offset inside the pool bo
  

•  */
  

• /* same reason as above */
  

  parser->const_ib.gpu_addr = parser->const_ib.sa_bo->soffset;

• r = radeon_ib_schedule(rdev, &parser->const_ib);
  

• if (r)
  

• 	goto out;
  

• r = radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib);
  

• } else {

• r = radeon_ib_schedule(rdev, &parser->ib, NULL);
  

  }

• parser->ib.vm_id = vm->id;
• /* ib pool is bind at 0 in virtual address space to gpu_addr is the

• * offset inside the pool bo
  

• */
  

• parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
• parser->ib.is_const_ib = false;
• r = radeon_ib_schedule(rdev, &parser->ib);

out: if (!r) { if (vm->fence) { diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 75cbe46..c48c354 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) radeon_fence_unref(&ib->fence); }

-int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,

•        struct radeon_ib *const_ib)
  

Since you are modifying an uncommented function, comments should be added, per the new documentation rules.

I don't mean to be picky, but there is no point having documentation rules if they aren't enforced.

{ struct radeon_ring *ring = &rdev->ring[ib->ring]; bool need_sync = false; @@ -105,6 +106,10 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) if (!need_sync) { radeon_semaphore_free(rdev, &ib->semaphore, NULL); }

• if (const_ib) {

• radeon_ring_ib_execute(rdev, const_ib->ring, const_ib);
  

• radeon_semaphore_free(rdev, &const_ib->semaphore, NULL);
  

• } radeon_ring_ib_execute(rdev, ib->ring, ib); r = radeon_fence_emit(rdev, &ib->fence, ib->ring); if (r) {

@@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) radeon_ring_unlock_undo(rdev, ring); return r; }

• if (const_ib) {

• const_ib->fence = radeon_fence_ref(ib->fence);
  

• } radeon_ring_unlock_commit(rdev, ring); return 0;

}

1.7.9.5

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Jul 17, 2012 at 5:50 AM, Christian König deathsimple@vodafone.de wrote:

On 13.07.2012 16:17, Tom Stellard wrote:

...

On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian König wrote:

...

Const IBs are executed on the CE not the CP, so we can't fence them in the normal way.

So submit them directly before the IB instead, just as the documentation says.

Signed-off-by: Christian König deathsimple@vodafone.de

drivers/gpu/drm/radeon/r100.c | 2 +- drivers/gpu/drm/radeon/r600.c | 2 +- drivers/gpu/drm/radeon/radeon.h | 3 ++- drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index e0f5ae8..4ee5a74 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[6] = PACKET2(0); ib.ptr[7] = PACKET2(0); ib.length_dw = 8;

•   r = radeon_ib_schedule(rdev, &ib);
  

•   r = radeon_ib_schedule(rdev, &ib, NULL);
    if (r) {
            radeon_scratch_free(rdev, scratch);
            radeon_ib_free(rdev, &ib);
  

diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 3156d25..c2e5069 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); ib.ptr[2] = 0xDEADBEEF; ib.length_dw = 3;

•   r = radeon_ib_schedule(rdev, &ib);
  

•   r = radeon_ib_schedule(rdev, &ib, NULL);
    if (r) {
            radeon_scratch_free(rdev, scratch);
            radeon_ib_free(rdev, &ib);
  

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 2cb355b..2d7f06c 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -751,7 +751,8 @@ struct si_rlc { int radeon_ib_get(struct radeon_device *rdev, int ring, struct radeon_ib *ib, unsigned size); void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib); +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,

•                  struct radeon_ib *const_ib);
  

  int radeon_ib_pool_init(struct radeon_device *rdev); void radeon_ib_pool_fini(struct radeon_device *rdev); int radeon_ib_ring_tests(struct radeon_device *rdev);

diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 553da67..d0be5d5 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser); parser->ib.vm_id = 0;

•   r = radeon_ib_schedule(rdev, &parser->ib);
  

•   r = radeon_ib_schedule(rdev, &parser->ib, NULL);
    if (r) {
            DRM_ERROR("Failed to schedule IB !\n");
    }
  

@@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser);

• parser->ib.vm_id = vm->id;
  

•   /* ib pool is bind at 0 in virtual address space,
  

•    * so gpu_addr is the offset inside the pool bo
  

•    */
  

•   parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
  

•   if ((rdev->family >= CHIP_TAHITI) &&
        (parser->chunk_const_ib_idx != -1)) {
            parser->const_ib.vm_id = vm->id;
  

•           /* ib pool is bind at 0 in virtual address space to
  

gpu_addr is the

•            * offset inside the pool bo
  

•            */
  

•           /* same reason as above */
            parser->const_ib.gpu_addr =
  

parser->const_ib.sa_bo->soffset;

•           r = radeon_ib_schedule(rdev, &parser->const_ib);
  

•           if (r)
  

•                   goto out;
  

•           r = radeon_ib_schedule(rdev, &parser->ib,
  

&parser->const_ib);

•   } else {
  

•           r = radeon_ib_schedule(rdev, &parser->ib, NULL);
    }
  

  • parser->ib.vm_id = vm->id;
    

•   /* ib pool is bind at 0 in virtual address space to gpu_addr is
  

the

•    * offset inside the pool bo
  

•    */
  

•   parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
  

•   parser->ib.is_const_ib = false;
  

•   r = radeon_ib_schedule(rdev, &parser->ib);
  

  out: if (!r) { if (vm->fence) {

diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 75cbe46..c48c354 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) radeon_fence_unref(&ib->fence); }

-int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,

•                  struct radeon_ib *const_ib)
  

Since you are modifying an uncommented function, comments should be added, per the new documentation rules.

I don't mean to be picky, but there is no point having documentation rules if they aren't enforced.

Oh no, please be picky about it. Otherwise I wouldn't learn it.

For this particular change Alex already had appropriate documentation in the pipeline and I wanted to rather change them instead of adding documentation in this patch.

Christian.

Still my earlier remark matter, i would rather not see cross comment reference it's confusing especially if code move. I rather see the same comment 2 times.

Cheers, Jerome

On Fri, Jul 13, 2012 at 10:08 AM, Christian König deathsimple@vodafone.de wrote:

Const IBs are executed on the CE not the CP, so we can't fence them in the normal way.

So submit them directly before the IB instead, just as the documentation says.

Signed-off-by: Christian König deathsimple@vodafone.de

drivers/gpu/drm/radeon/r100.c | 2 +- drivers/gpu/drm/radeon/r600.c | 2 +- drivers/gpu/drm/radeon/radeon.h | 3 ++- drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index e0f5ae8..4ee5a74 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[6] = PACKET2(0); ib.ptr[7] = PACKET2(0); ib.length_dw = 8;

•   r = radeon_ib_schedule(rdev, &ib);
  

•   r = radeon_ib_schedule(rdev, &ib, NULL);
    if (r) {
            radeon_scratch_free(rdev, scratch);
            radeon_ib_free(rdev, &ib);
  

diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 3156d25..c2e5069 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); ib.ptr[2] = 0xDEADBEEF; ib.length_dw = 3;

•   r = radeon_ib_schedule(rdev, &ib);
  

•   r = radeon_ib_schedule(rdev, &ib, NULL);
    if (r) {
            radeon_scratch_free(rdev, scratch);
            radeon_ib_free(rdev, &ib);
  

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 2cb355b..2d7f06c 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -751,7 +751,8 @@ struct si_rlc { int radeon_ib_get(struct radeon_device *rdev, int ring, struct radeon_ib *ib, unsigned size); void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib); +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,

•                  struct radeon_ib *const_ib);
  

int radeon_ib_pool_init(struct radeon_device *rdev); void radeon_ib_pool_fini(struct radeon_device *rdev); int radeon_ib_ring_tests(struct radeon_device *rdev); diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 553da67..d0be5d5 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser); parser->ib.vm_id = 0;

•   r = radeon_ib_schedule(rdev, &parser->ib);
  

•   r = radeon_ib_schedule(rdev, &parser->ib, NULL);
    if (r) {
            DRM_ERROR("Failed to schedule IB !\n");
    }
  

@@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser);

•   parser->ib.vm_id = vm->id;
  

•   /* ib pool is bind at 0 in virtual address space,
  

•    * so gpu_addr is the offset inside the pool bo
  

•    */
  

•   parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
  

•   if ((rdev->family >= CHIP_TAHITI) &&
        (parser->chunk_const_ib_idx != -1)) {
            parser->const_ib.vm_id = vm->id;
  

•           /* ib pool is bind at 0 in virtual address space to gpu_addr is the
  

•            * offset inside the pool bo
  

•            */
  

•           /* same reason as above */
  

Don't remove comment, code might move and the above comment might not be the same better to duplicate comment then trying to cross reference comment across file.

            parser->const_ib.gpu_addr = parser->const_ib.sa_bo->soffset;

•           r = radeon_ib_schedule(rdev, &parser->const_ib);
  

•           if (r)
  

•                   goto out;
  

•           r = radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib);
  

•   } else {
  

•           r = radeon_ib_schedule(rdev, &parser->ib, NULL);
    }
  
  

•   parser->ib.vm_id = vm->id;
  

•   /* ib pool is bind at 0 in virtual address space to gpu_addr is the
  

•    * offset inside the pool bo
  

•    */
  

•   parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
  

•   parser->ib.is_const_ib = false;
  

•   r = radeon_ib_schedule(rdev, &parser->ib);
  

out: if (!r) { if (vm->fence) { diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 75cbe46..c48c354 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) radeon_fence_unref(&ib->fence); }

-int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,

•                  struct radeon_ib *const_ib)
  

{ struct radeon_ring *ring = &rdev->ring[ib->ring]; bool need_sync = false; @@ -105,6 +106,10 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) if (!need_sync) { radeon_semaphore_free(rdev, &ib->semaphore, NULL); }

•   if (const_ib) {
  

•           radeon_ring_ib_execute(rdev, const_ib->ring, const_ib);
  

•           radeon_semaphore_free(rdev, &const_ib->semaphore, NULL);
  

•   }
    radeon_ring_ib_execute(rdev, ib->ring, ib);
    r = radeon_fence_emit(rdev, &ib->fence, ib->ring);
    if (r) {
  

@@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) radeon_ring_unlock_undo(rdev, ring); return r; }

•   if (const_ib) {
  

•           const_ib->fence = radeon_fence_ref(ib->fence);
  

•   }
    radeon_ring_unlock_commit(rdev, ring);
    return 0;
  

}

1.7.9.5

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On 16.07.2012 23:14, alexdeucher@gmail.com wrote:

From: Alex Deucher alexander.deucher@amd.com

When submitting a CONST_IB, emit a SWITCH_BUFFER packet before the CONST_IB. This isn't strictly necessary (the driver will work fine without it), but is good practice and allows for more flexible DE/CE sychronization options in the future. Current userspace drivers do not take advantage of the CE yet.

Signed-off-by: Alex Deucher alexander.deucher@amd.com

drivers/gpu/drm/radeon/si.c | 6 ++++++ drivers/gpu/drm/radeon/sid.h | 1 + 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index 53e313b..191a3cd 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -1778,6 +1778,12 @@ void si_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib) else header = PACKET3(PACKET3_INDIRECT_BUFFER, 2);

• if (ib->is_const_ib) {

• /* set switch buffer packet before const IB */
  

• radeon_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
  

• radeon_ring_write(ring, 0);
  

• }

Additional to that I don't think the read cache flush and rptr saving is appropriate for a const IB, but on the other side I don't claim that I have understood the CE/DE separation fully yet.

Christian.

radeon_ring_write(ring, header); radeon_ring_write(ring, #ifdef __BIG_ENDIAN diff --git a/drivers/gpu/drm/radeon/sid.h b/drivers/gpu/drm/radeon/sid.h index db40679..7869089 100644 --- a/drivers/gpu/drm/radeon/sid.h +++ b/drivers/gpu/drm/radeon/sid.h @@ -901,5 +901,6 @@ #define PACKET3_WAIT_ON_DE_COUNTER_DIFF 0x88 #define PACKET3_SET_CE_DE_COUNTERS 0x89 #define PACKET3_WAIT_ON_AVAIL_BUFFER 0x8A +#define PACKET3_SWITCH_BUFFER 0x8B

#endif

From: Alex Deucher alexander.deucher@amd.com

When submitting a CONST_IB, emit a SWITCH_BUFFER packet before the CONST_IB. This isn't strictly necessary (the driver will work fine without it), but is good practice and allows for more flexible DE/CE sychronization options in the future. Current userspace drivers do not take advantage of the CE yet.

v2: - clean up code flow a bit - no need to flush caches for CONST IB

Signed-off-by: Alex Deucher alexander.deucher@amd.com --- drivers/gpu/drm/radeon/si.c | 49 ++++++++++++++++++++++++------------------ drivers/gpu/drm/radeon/sid.h | 1 + 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index 53e313b..2b12cae 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -1765,18 +1765,23 @@ void si_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib) struct radeon_ring *ring = &rdev->ring[ib->ring]; u32 header;

- if (ring->rptr_save_reg) { - uint32_t next_rptr = ring->wptr + 3 + 4 + 8; - radeon_ring_write(ring, PACKET3(PACKET3_SET_CONFIG_REG, 1)); - radeon_ring_write(ring, ((ring->rptr_save_reg - - PACKET3_SET_CONFIG_REG_START) >> 2)); - radeon_ring_write(ring, next_rptr); - } + if (ib->is_const_ib) { + /* set switch buffer packet before const IB */ + radeon_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0)); + radeon_ring_write(ring, 0);

- if (ib->is_const_ib) header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2); - else + } else { + if (ring->rptr_save_reg) { + uint32_t next_rptr = ring->wptr + 3 + 4 + 8; + radeon_ring_write(ring, PACKET3(PACKET3_SET_CONFIG_REG, 1)); + radeon_ring_write(ring, ((ring->rptr_save_reg - + PACKET3_SET_CONFIG_REG_START) >> 2)); + radeon_ring_write(ring, next_rptr); + } + header = PACKET3(PACKET3_INDIRECT_BUFFER, 2); + }

radeon_ring_write(ring, header); radeon_ring_write(ring, @@ -1787,18 +1792,20 @@ void si_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib) radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); radeon_ring_write(ring, ib->length_dw | (ib->vm_id << 24));

- /* flush read cache over gart for this vmid */ - radeon_ring_write(ring, PACKET3(PACKET3_SET_CONFIG_REG, 1)); - radeon_ring_write(ring, (CP_COHER_CNTL2 - PACKET3_SET_CONFIG_REG_START) >> 2); - radeon_ring_write(ring, ib->vm_id); - radeon_ring_write(ring, PACKET3(PACKET3_SURFACE_SYNC, 3)); - radeon_ring_write(ring, PACKET3_TCL1_ACTION_ENA | - PACKET3_TC_ACTION_ENA | - PACKET3_SH_KCACHE_ACTION_ENA | - PACKET3_SH_ICACHE_ACTION_ENA); - radeon_ring_write(ring, 0xFFFFFFFF); - radeon_ring_write(ring, 0); - radeon_ring_write(ring, 10); /* poll interval */ + if (!ib->is_const_ib) { + /* flush read cache over gart for this vmid */ + radeon_ring_write(ring, PACKET3(PACKET3_SET_CONFIG_REG, 1)); + radeon_ring_write(ring, (CP_COHER_CNTL2 - PACKET3_SET_CONFIG_REG_START) >> 2); + radeon_ring_write(ring, ib->vm_id); + radeon_ring_write(ring, PACKET3(PACKET3_SURFACE_SYNC, 3)); + radeon_ring_write(ring, PACKET3_TCL1_ACTION_ENA | + PACKET3_TC_ACTION_ENA | + PACKET3_SH_KCACHE_ACTION_ENA | + PACKET3_SH_ICACHE_ACTION_ENA); + radeon_ring_write(ring, 0xFFFFFFFF); + radeon_ring_write(ring, 0); + radeon_ring_write(ring, 10); /* poll interval */ + } }

/* diff --git a/drivers/gpu/drm/radeon/sid.h b/drivers/gpu/drm/radeon/sid.h index db40679..7869089 100644 --- a/drivers/gpu/drm/radeon/sid.h +++ b/drivers/gpu/drm/radeon/sid.h @@ -901,5 +901,6 @@ #define PACKET3_WAIT_ON_DE_COUNTER_DIFF 0x88 #define PACKET3_SET_CE_DE_COUNTERS 0x89 #define PACKET3_WAIT_ON_AVAIL_BUFFER 0x8A +#define PACKET3_SWITCH_BUFFER 0x8B

#endif