[kernel PATCH] drm/nouveau: add NOUVEAU_GEM_RELOC_HILO

[kernel PATCH] drm/nouveau: add NOUVEAU_GEM_RELOC_HILO

[Marcin Slusarz]

add new relocation flag which allows to halving number of relocation
entries on >= nv50 cards

Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/nouveau/nouveau_gem.c |   22 +++++++++++++++-------
 include/drm/nouveau_drm.h             |    1 +
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 9a1fdcf..2e21412 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -507,6 +507,7 @@ nouveau_gem_pushbuf_reloc_apply(struct drm_device *dev,
 		struct drm_nouveau_gem_pushbuf_bo *b;
 		struct nouveau_bo *nvbo;
 		uint32_t data;
+		uint32_t data2;
 
 		if (unlikely(r->bo_index > req->nr_buffers)) {
 			NV_ERROR(dev, "reloc bo index invalid\n");
@@ -544,17 +545,22 @@ nouveau_gem_pushbuf_reloc_apply(struct drm_device *dev,
 
 		if (r->flags & NOUVEAU_GEM_RELOC_LOW)
 			data = b->presumed.offset + r->data;
-		else
-		if (r->flags & NOUVEAU_GEM_RELOC_HIGH)
+		else if (r->flags & NOUVEAU_GEM_RELOC_HIGH)
 			data = (b->presumed.offset + r->data) >> 32;
-		else
+		else if (r->flags & NOUVEAU_GEM_RELOC_HILO) {
+			data = (b->presumed.offset + r->data) >> 32;
+			data2 = b->presumed.offset + r->data;
+		} else
 			data = r->data;
 
 		if (r->flags & NOUVEAU_GEM_RELOC_OR) {
-			if (b->presumed.domain == NOUVEAU_GEM_DOMAIN_GART)
-				data |= r->tor;
-			else
-				data |= r->vor;
+			if (b->presumed.domain == NOUVEAU_GEM_DOMAIN_GART) {
+				data  |= r->tor;
+				data2 |= r->tor;
+			} else {
+				data  |= r->vor;
+				data2 |= r->vor;
+			}
 		}
 
 		spin_lock(&nvbo->bo.lock);
@@ -566,6 +572,8 @@ nouveau_gem_pushbuf_reloc_apply(struct drm_device *dev,
 		}
 
 		nouveau_bo_wr32(nvbo, r->reloc_bo_offset >> 2, data);
+		if (r->flags & NOUVEAU_GEM_RELOC_HILO)
+			nouveau_bo_wr32(nvbo, (r->reloc_bo_offset >> 2) + 1, data2);
 	}
 
 	kfree(reloc);
diff --git a/include/drm/nouveau_drm.h b/include/drm/nouveau_drm.h
index bc5590b..126ea8a 100644
--- a/include/drm/nouveau_drm.h
+++ b/include/drm/nouveau_drm.h
@@ -137,6 +137,7 @@ struct drm_nouveau_gem_pushbuf_bo {
 #define NOUVEAU_GEM_RELOC_LOW  (1 << 0)
 #define NOUVEAU_GEM_RELOC_HIGH (1 << 1)
 #define NOUVEAU_GEM_RELOC_OR   (1 << 2)
+#define NOUVEAU_GEM_RELOC_HILO (1 << 3)
 #define NOUVEAU_GEM_MAX_RELOCS 1024
 struct drm_nouveau_gem_pushbuf_reloc {
 	uint32_t reloc_bo_index;
-- 
1.7.3.3

[libdrm PATCH] nouveau: optimize OUT_RELOC2

[Marcin Slusarz]

use one relocation with newly introduced flag

unfortunately we can't enable it unconditionally, because we don't
know if the kernel supports it...
---
 include/drm/nouveau_drm.h |    1 +
 nouveau/nouveau_bo.h      |    1 +
 nouveau/nouveau_pushbuf.h |    9 +++++++++
 nouveau/nouveau_reloc.c   |   28 ++++++++++++++++++++++------
 4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/include/drm/nouveau_drm.h b/include/drm/nouveau_drm.h
index b18cad0..4c35399 100644
--- a/include/drm/nouveau_drm.h
+++ b/include/drm/nouveau_drm.h
@@ -138,6 +138,7 @@ struct drm_nouveau_gem_pushbuf_bo {
 #define NOUVEAU_GEM_RELOC_LOW  (1 << 0)
 #define NOUVEAU_GEM_RELOC_HIGH (1 << 1)
 #define NOUVEAU_GEM_RELOC_OR   (1 << 2)
+#define NOUVEAU_GEM_RELOC_HILO (1 << 3)
 #define NOUVEAU_GEM_MAX_RELOCS 1024
 struct drm_nouveau_gem_pushbuf_reloc {
 	uint32_t reloc_bo_index;
diff --git a/nouveau/nouveau_bo.h b/nouveau/nouveau_bo.h
index 3a1f2d4..bbf88ec 100644
--- a/nouveau/nouveau_bo.h
+++ b/nouveau/nouveau_bo.h
@@ -37,6 +37,7 @@
 #define NOUVEAU_BO_NOSYNC (1 << 13)
 #define NOUVEAU_BO_NOWAIT (1 << 14)
 #define NOUVEAU_BO_IFLUSH (1 << 15)
+#define NOUVEAU_BO_HILO   (1 << 16)
 #define NOUVEAU_BO_DUMMY  (1 << 31)
 
 #define NOUVEAU_BO_TILE_LAYOUT_MASK 0x0000ff00
diff --git a/nouveau/nouveau_pushbuf.h b/nouveau/nouveau_pushbuf.h
index f256c70..92bffde 100644
--- a/nouveau/nouveau_pushbuf.h
+++ b/nouveau/nouveau_pushbuf.h
@@ -142,15 +142,24 @@ OUT_RELOCh(struct nouveau_channel *chan, struct nouveau_bo *bo,
 	return OUT_RELOC(chan, bo, delta, flags | NOUVEAU_BO_HIGH, 0, 0);
 }
 
+//disabled for now
+//#define NOUVEAU_DRM_NEW_RELOC
 /* High 32-bits, then low 32-bits of offset */
 static __inline__ int
 OUT_RELOC2(struct nouveau_channel *chan, struct nouveau_bo *bo,
 	   unsigned delta, unsigned flags)
 {
+#ifdef NOUVEAU_DRM_NEW_RELOC
+	int r = nouveau_pushbuf_emit_reloc(chan, chan->cur, bo,
+					  delta, flags | NOUVEAU_BO_HILO, 0, 0);
+	chan->cur += 2;
+	return r;
+#else
 	int r = OUT_RELOC(chan, bo, delta, flags | NOUVEAU_BO_HIGH, 0, 0);
 	if (r)
 		return r;
 	return OUT_RELOC(chan, bo, delta, flags | NOUVEAU_BO_LOW, 0, 0);
+#endif
 }
 
 #endif
diff --git a/nouveau/nouveau_reloc.c b/nouveau/nouveau_reloc.c
index c636333..864ab61 100644
--- a/nouveau/nouveau_reloc.c
+++ b/nouveau/nouveau_reloc.c
@@ -29,19 +29,21 @@
 
 static uint32_t
 nouveau_reloc_calc(struct drm_nouveau_gem_pushbuf_bo *pbbo,
-		   struct drm_nouveau_gem_pushbuf_reloc *r)
+		   struct drm_nouveau_gem_pushbuf_reloc *r,
+		   uint32_t flags)
 {
 	uint32_t push = 0;
+	flags |= r->flags;
 
-	if (r->flags & NOUVEAU_GEM_RELOC_LOW)
+	if (flags & NOUVEAU_GEM_RELOC_LOW)
 		push = (pbbo->presumed.offset + r->data);
 	else
-	if (r->flags & NOUVEAU_GEM_RELOC_HIGH)
+	if (flags & NOUVEAU_GEM_RELOC_HIGH)
 		push = (pbbo->presumed.offset + r->data) >> 32;
 	else
 		push = r->data;
 
-	if (r->flags & NOUVEAU_GEM_RELOC_OR) {
+	if (flags & NOUVEAU_GEM_RELOC_OR) {
 		if (pbbo->presumed.domain & NOUVEAU_GEM_DOMAIN_VRAM)
 			push |= r->vor;
 		else
@@ -138,15 +140,29 @@ nouveau_reloc_emit(struct nouveau_channel *chan, struct nouveau_bo *reloc_bo,
 		r->flags |= NOUVEAU_GEM_RELOC_HIGH;
 	if (flags & NOUVEAU_BO_OR)
 		r->flags |= NOUVEAU_GEM_RELOC_OR;
+	if (flags & NOUVEAU_BO_HILO)
+		r->flags |= NOUVEAU_GEM_RELOC_HILO;
 	r->data = data;
 	r->vor = vor;
 	r->tor = tor;
 
 	if (reloc_ptr) {
 		if (flags & NOUVEAU_BO_DUMMY)
-			*reloc_ptr = 0;
+			if (flags & NOUVEAU_BO_HILO) {
+				reloc_ptr[0] = 0;
+				reloc_ptr[1] = 0;
+			} else 
+				*reloc_ptr = 0;
 		else
-			*reloc_ptr = nouveau_reloc_calc(pbbo, r);
+		{
+			if (flags & NOUVEAU_BO_HILO) {
+				reloc_ptr[0] = nouveau_reloc_calc(pbbo, r,
+							NOUVEAU_GEM_RELOC_HIGH);
+				reloc_ptr[1] = nouveau_reloc_calc(pbbo, r,
+							NOUVEAU_GEM_RELOC_LOW);
+			} else
+				*reloc_ptr = nouveau_reloc_calc(pbbo, r, 0);
+		}
 	}
 
 	return 0;
-- 
1.7.3.3

[mesa PATCH] gallium/nouveau: optimize so_reloc2

[Marcin Slusarz]

use one relocation with newly introduced flag

unfortunately we can't enable it unconditionally, because we don't
know if the kernel supports it...
---
 src/gallium/drivers/nouveau/nouveau_stateobj.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nouveau_stateobj.h b/src/gallium/drivers/nouveau/nouveau_stateobj.h
index be8850b..4a08ba3 100644
--- a/src/gallium/drivers/nouveau/nouveau_stateobj.h
+++ b/src/gallium/drivers/nouveau/nouveau_stateobj.h
@@ -182,6 +182,9 @@ so_method(struct nouveau_stateobj *so, struct nouveau_grobj *gr,
 	so->cur = 0;
 }
 
+//disabled for now
+//#define NOUVEAU_MESA_NEW_RELOC
+
 static INLINE void
 so_reloc(struct nouveau_stateobj *so, struct nouveau_bo *bo,
 	 unsigned data, unsigned flags, unsigned vor, unsigned tor)
@@ -208,6 +211,10 @@ so_reloc(struct nouveau_stateobj *so, struct nouveau_bo *bo,
 	r[so->cur_reloc].tor = tor;
 
 	so_data(so, data);
+#if defined(NOUVEAU_MESA_NEW_RELOC) && defined(NOUVEAU_BO_HILO)
+	if (flags & NOUVEAU_BO_HILO)
+		so_data(so, data);
+#endif
 	so->cur_reloc++;
 }
 
@@ -215,8 +222,12 @@ static INLINE void
 so_reloc2(struct nouveau_stateobj *so, struct nouveau_bo *bo,
 	 unsigned data, unsigned flags, unsigned vor, unsigned tor)
 {
+#if defined(NOUVEAU_MESA_NEW_RELOC) && defined(NOUVEAU_BO_HILO)
+	so_reloc(so, bo, data, flags | NOUVEAU_BO_HILO, vor, tor);
+#else
 	so_reloc(so, bo, data, flags | NOUVEAU_BO_HIGH, vor, tor);
 	so_reloc(so, bo, data, flags | NOUVEAU_BO_LOW, vor, tor);
+#endif
 }
 
 /* Determine if this buffer object is referenced by this state object. */
-- 
1.7.3.3

Re: [kernel PATCH] drm/nouveau: add NOUVEAU_GEM_RELOC_HILO

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 3291 bytes --]

Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> add new relocation flag which allows to halving number of relocation
> entries on >= nv50 cards
>
I suspect that this change is unnecessary. If you're actually seeing a
performance improvement (I guess you are?), it's very likely to be
masking the actual root of the problem -- last time I checked the nv50
gallium pipe driver was rather careless about reloc re-submission (it
re-submits the whole reloc chain once for every draw call instead of
once for every pushbuf flush), and nouveau_reloc_emit() and friends are
very unoptimized.

I'm betting that using HILO or not will make pretty much no difference
once those two places are fixed :).

> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpu/drm/nouveau/nouveau_gem.c |   22 +++++++++++++++-------
>  include/drm/nouveau_drm.h             |    1 +
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 9a1fdcf..2e21412 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -507,6 +507,7 @@ nouveau_gem_pushbuf_reloc_apply(struct drm_device *dev,
>  		struct drm_nouveau_gem_pushbuf_bo *b;
>  		struct nouveau_bo *nvbo;
>  		uint32_t data;
> +		uint32_t data2;
>  
>  		if (unlikely(r->bo_index > req->nr_buffers)) {
>  			NV_ERROR(dev, "reloc bo index invalid\n");
> @@ -544,17 +545,22 @@ nouveau_gem_pushbuf_reloc_apply(struct drm_device *dev,
>  
>  		if (r->flags & NOUVEAU_GEM_RELOC_LOW)
>  			data = b->presumed.offset + r->data;
> -		else
> -		if (r->flags & NOUVEAU_GEM_RELOC_HIGH)
> +		else if (r->flags & NOUVEAU_GEM_RELOC_HIGH)
>  			data = (b->presumed.offset + r->data) >> 32;
> -		else
> +		else if (r->flags & NOUVEAU_GEM_RELOC_HILO) {
> +			data = (b->presumed.offset + r->data) >> 32;
> +			data2 = b->presumed.offset + r->data;
> +		} else
>  			data = r->data;
>  
>  		if (r->flags & NOUVEAU_GEM_RELOC_OR) {
> -			if (b->presumed.domain == NOUVEAU_GEM_DOMAIN_GART)
> -				data |= r->tor;
> -			else
> -				data |= r->vor;
> +			if (b->presumed.domain == NOUVEAU_GEM_DOMAIN_GART) {
> +				data  |= r->tor;
> +				data2 |= r->tor;
> +			} else {
> +				data  |= r->vor;
> +				data2 |= r->vor;
> +			}
>  		}
>  
>  		spin_lock(&nvbo->bo.lock);
> @@ -566,6 +572,8 @@ nouveau_gem_pushbuf_reloc_apply(struct drm_device *dev,
>  		}
>  
>  		nouveau_bo_wr32(nvbo, r->reloc_bo_offset >> 2, data);
> +		if (r->flags & NOUVEAU_GEM_RELOC_HILO)
> +			nouveau_bo_wr32(nvbo, (r->reloc_bo_offset >> 2) + 1, data2);
>  	}
>  
>  	kfree(reloc);
> diff --git a/include/drm/nouveau_drm.h b/include/drm/nouveau_drm.h
> index bc5590b..126ea8a 100644
> --- a/include/drm/nouveau_drm.h
> +++ b/include/drm/nouveau_drm.h
> @@ -137,6 +137,7 @@ struct drm_nouveau_gem_pushbuf_bo {
>  #define NOUVEAU_GEM_RELOC_LOW  (1 << 0)
>  #define NOUVEAU_GEM_RELOC_HIGH (1 << 1)
>  #define NOUVEAU_GEM_RELOC_OR   (1 << 2)
> +#define NOUVEAU_GEM_RELOC_HILO (1 << 3)
>  #define NOUVEAU_GEM_MAX_RELOCS 1024
>  struct drm_nouveau_gem_pushbuf_reloc {
>  	uint32_t reloc_bo_index;

[-- Attachment #1.2: Type: application/pgp-signature, Size: 229 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [kernel PATCH] drm/nouveau: add NOUVEAU_GEM_RELOC_HILO

[Marcin Slusarz]

On Wed, Jan 12, 2011 at 10:08:55PM +0100, Francisco Jerez wrote:
> Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> 
> > add new relocation flag which allows to halving number of relocation
> > entries on >= nv50 cards
> >
> I suspect that this change is unnecessary. If you're actually seeing a
> performance improvement (I guess you are?), it's very likely to be

I did it for performance reasons, but actual improvement is very small (0.3%).
I sent these patches, because it's just better to do less and use less memory.

> masking the actual root of the problem -- last time I checked the nv50
> gallium pipe driver was rather careless about reloc re-submission (it
> re-submits the whole reloc chain once for every draw call instead of
> once for every pushbuf flush), and nouveau_reloc_emit() and friends are
> very unoptimized.
> 
> I'm betting that using HILO or not will make pretty much no difference
> once those two places are fixed :).

It looks they are already fixed :).

Well, you can treat these patches as data (relocations are not a bottleneck)
and forget about them.

Marcin