[PATCH 1/3] drm/radeon: return an error if there is nothing to wait for

dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 1/3] drm/radeon: return an error if there is nothing to wait for
@ 2012-07-13 14:08 Christian König
  2012-07-13 14:08 ` [PATCH 2/3] drm/radeon: let sa manager block for fences " Christian König
  2012-07-13 14:08 ` [PATCH 3/3] drm/radeon: fix const IB handling Christian König
  0 siblings, 2 replies; 13+ messages in thread
From: Christian König @ 2012-07-13 14:08 UTC (permalink / raw)
  To: dri-devel

Otherwise the sa managers out of memory
handling doesn't work.

Signed-off-by: Christian König <deathsimple@vodafone.de>
---
 drivers/gpu/drm/radeon/radeon_fence.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
index 76c5b22..7a181c3 100644
--- a/drivers/gpu/drm/radeon/radeon_fence.c
+++ b/drivers/gpu/drm/radeon/radeon_fence.c
@@ -331,7 +331,7 @@ static int radeon_fence_wait_any_seq(struct radeon_device *rdev,
 
 	/* nothing to wait for ? */
 	if (ring == RADEON_NUM_RINGS) {
-		return 0;
+		return -ENOENT;
 	}
 
 	while (!radeon_fence_any_seq_signaled(rdev, target_seq)) {
-- 
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] 13+ messages in thread

* [PATCH 2/3] drm/radeon: let sa manager block for fences to wait for
  2012-07-13 14:08 [PATCH 1/3] drm/radeon: return an error if there is nothing to wait for Christian König
@ 2012-07-13 14:08 ` Christian König
  2012-07-13 14:14   ` Tom Stellard
  2012-07-13 14:08 ` [PATCH 3/3] drm/radeon: fix const IB handling Christian König
  1 sibling, 1 reply; 13+ messages in thread
From: Christian König @ 2012-07-13 14:08 UTC (permalink / raw)
  To: dri-devel

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;
+}
+
 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] drm/radeon: let sa manager block for fences to wait for
  2012-07-13 14:08 ` [PATCH 2/3] drm/radeon: let sa manager block for fences " Christian König
@ 2012-07-13 14:14   ` Tom Stellard
  0 siblings, 0 replies; 13+ messages in thread
From: Tom Stellard @ 2012-07-13 14:14 UTC (permalink / raw)
  To: Christian König; +Cc: dri-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] drm/radeon: fix const IB handling
  2012-07-13 14:08 [PATCH 1/3] drm/radeon: return an error if there is nothing to wait for Christian König
  2012-07-13 14:08 ` [PATCH 2/3] drm/radeon: let sa manager block for fences " Christian König
@ 2012-07-13 14:08 ` Christian König
  2012-07-13 14:17   ` Tom Stellard
                     ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Christian König @ 2012-07-13 14:08 UTC (permalink / raw)
  To: dri-devel

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)
 {
 	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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] drm/radeon: fix const IB handling
  2012-07-13 14:08 ` [PATCH 3/3] drm/radeon: fix const IB handling Christian König
@ 2012-07-13 14:17   ` Tom Stellard
  2012-07-17  9:50     ` Christian König
  2012-07-13 14:53   ` Jerome Glisse
  2012-07-16 21:14   ` [PATCH] drm/radeon: update ib_execute for SI alexdeucher
  2 siblings, 1 reply; 13+ messages in thread
From: Tom Stellard @ 2012-07-13 14:17 UTC (permalink / raw)
  To: Christian König; +Cc: 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] drm/radeon: fix const IB handling
  2012-07-13 14:17   ` Tom Stellard
@ 2012-07-17  9:50     ` Christian König
  2012-07-17 14:25       ` Jerome Glisse
  0 siblings, 1 reply; 13+ messages in thread
From: Christian König @ 2012-07-17  9:50 UTC (permalink / raw)
  To: Tom Stellard; +Cc: dri-devel

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.

>
>>   {
>>   	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
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] drm/radeon: fix const IB handling
  2012-07-17  9:50     ` Christian König
@ 2012-07-17 14:25       ` Jerome Glisse
  2012-07-17 14:32         ` Christian König
  0 siblings, 1 reply; 13+ messages in thread
From: Jerome Glisse @ 2012-07-17 14:25 UTC (permalink / raw)
  To: Christian König; +Cc: 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] drm/radeon: fix const IB handling
  2012-07-17 14:25       ` Jerome Glisse
@ 2012-07-17 14:32         ` Christian König
  0 siblings, 0 replies; 13+ messages in thread
From: Christian König @ 2012-07-17 14:32 UTC (permalink / raw)
  To: Jerome Glisse; +Cc: dri-devel

On 17.07.2012 16:25, Jerome Glisse wrote:
> 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.
That was in the other patch, and yeah your right I changed that one.

Christian.

>
> Cheers,
> Jerome
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] drm/radeon: fix const IB handling
  2012-07-13 14:08 ` [PATCH 3/3] drm/radeon: fix const IB handling Christian König
  2012-07-13 14:17   ` Tom Stellard
@ 2012-07-13 14:53   ` Jerome Glisse
  2012-07-16 21:14   ` [PATCH] drm/radeon: update ib_execute for SI alexdeucher
  2 siblings, 0 replies; 13+ messages in thread
From: Jerome Glisse @ 2012-07-13 14:53 UTC (permalink / raw)
  To: Christian König; +Cc: dri-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/radeon: update ib_execute for SI
  2012-07-13 14:08 ` [PATCH 3/3] drm/radeon: fix const IB handling Christian König
  2012-07-13 14:17   ` Tom Stellard
  2012-07-13 14:53   ` Jerome Glisse
@ 2012-07-16 21:14   ` alexdeucher
  2012-07-17  9:59     ` Christian König
  2 siblings, 1 reply; 13+ messages in thread
From: alexdeucher @ 2012-07-16 21:14 UTC (permalink / raw)
  To: airlied, dri-devel; +Cc: Alex Deucher

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);
+	}
+
 	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
-- 
1.7.7.5

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/radeon: update ib_execute for SI
  2012-07-16 21:14   ` [PATCH] drm/radeon: update ib_execute for SI alexdeucher
@ 2012-07-17  9:59     ` Christian König
  2012-07-17 12:33       ` Alex Deucher
  0 siblings, 1 reply; 13+ messages in thread
From: Christian König @ 2012-07-17  9:59 UTC (permalink / raw)
  To: alexdeucher; +Cc: Alex Deucher, 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/radeon: update ib_execute for SI
  2012-07-17  9:59     ` Christian König
@ 2012-07-17 12:33       ` Alex Deucher
  2012-07-17 16:37         ` [PATCH] drm/radeon: update ib_execute for SI (v2) alexdeucher
  0 siblings, 1 reply; 13+ messages in thread
From: Alex Deucher @ 2012-07-17 12:33 UTC (permalink / raw)
  To: Christian König; +Cc: Alex Deucher, dri-devel

On Tue, Jul 17, 2012 at 5:59 AM, Christian König
<deathsimple@vodafone.de> wrote:
> 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.

Yeah, we could probably skip them for the const ib.

Alex

>
> 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
>
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/radeon: update ib_execute for SI (v2)
  2012-07-17 12:33       ` Alex Deucher
@ 2012-07-17 16:37         ` alexdeucher
  0 siblings, 0 replies; 13+ messages in thread
From: alexdeucher @ 2012-07-17 16:37 UTC (permalink / raw)
  To: airlied, dri-devel; +Cc: Alex Deucher

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
-- 
1.7.7.5

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-07-17 16:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-13 14:08 [PATCH 1/3] drm/radeon: return an error if there is nothing to wait for Christian König
2012-07-13 14:08 ` [PATCH 2/3] drm/radeon: let sa manager block for fences " Christian König
2012-07-13 14:14   ` Tom Stellard
2012-07-13 14:08 ` [PATCH 3/3] drm/radeon: fix const IB handling Christian König
2012-07-13 14:17   ` Tom Stellard
2012-07-17  9:50     ` Christian König
2012-07-17 14:25       ` Jerome Glisse
2012-07-17 14:32         ` Christian König
2012-07-13 14:53   ` Jerome Glisse
2012-07-16 21:14   ` [PATCH] drm/radeon: update ib_execute for SI alexdeucher
2012-07-17  9:59     ` Christian König
2012-07-17 12:33       ` Alex Deucher
2012-07-17 16:37         ` [PATCH] drm/radeon: update ib_execute for SI (v2) alexdeucher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).