Re: [Mesa-dev] [PATCH 4/5] nvc0: avoid using magic numbers for the uniform_bo offsets

2016-03-19 Thread Samuel Pitoiset 



On 03/17/2016 01:02 AM, Pierre Moreau wrote:

Hello,

On 09:55 PM - Mar 15 2016, Samuel Pitoiset wrote:

Instead make use of constants to improve readability.

Signed-off-by: Samuel Pitoiset 
---
  src/gallium/drivers/nouveau/nvc0/nvc0_compute.c| 13 +-
  src/gallium/drivers/nouveau/nvc0/nvc0_context.h| 22 +
  src/gallium/drivers/nouveau/nvc0/nvc0_program.c| 12 +-
  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 12 +-
  src/gallium/drivers/nouveau/nvc0/nvc0_screen.h |  2 +-
  .../drivers/nouveau/nvc0/nvc0_state_validate.c | 28 --
  src/gallium/drivers/nouveau/nvc0/nvc0_tex.c|  9 ---
  src/gallium/drivers/nouveau/nvc0/nvc0_vbo.c| 14 ++-
  8 files changed, 69 insertions(+), 43 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c 
b/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
index ffbb16f..6aaa7ce 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
@@ -153,7 +153,7 @@ nvc0_compute_validate_constbufs(struct nvc0_context *nvc0)

if (nvc0->constbuf[s][i].user) {
   struct nouveau_bo *bo = nvc0->screen->uniform_bo;
- const unsigned base = s << 16;
+ const unsigned base = NVC0_CB_USR_INFO(s);
   const unsigned size = nvc0->constbuf[s][0].size;
   assert(i == 0); /* we really only want OpenGL uniforms here */
   assert(nvc0->constbuf[s][0].u.data);
@@ -207,8 +207,8 @@ nvc0_compute_validate_driverconst(struct nvc0_context *nvc0)

 BEGIN_NVC0(push, NVC0_CP(CB_SIZE), 3);
 PUSH_DATA (push, 1024);
-   PUSH_DATAh(push, screen->uniform_bo->offset + (6 << 16) + (5 << 10));
-   PUSH_DATA (push, screen->uniform_bo->offset + (6 << 16) + (5 << 10));
+   PUSH_DATAh(push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(5));
+   PUSH_DATA (push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(5));
 BEGIN_NVC0(push, NVC0_CP(CB_BIND), 1);
 PUSH_DATA (push, (15 << 8) | 1);

@@ -219,15 +219,16 @@ static void
  nvc0_compute_validate_buffers(struct nvc0_context *nvc0)
  {
 struct nouveau_pushbuf *push = nvc0->base.pushbuf;
+   struct nvc0_screen *screen = nvc0->screen;


Why define `screen` and not `offset`? You would *save* more characters. It's
not like you are reusing screen otherwise in this function. Sure the same
pattern comes back multiple times in various functions, but you should be able
to do the same factorisation in those as well.


Because I prefer to see PUSH_DATA with uniform_bo with git grep. :-)
That might help.




 const int s = 5;
 int i;

 BEGIN_NVC0(push, NVC0_CP(CB_SIZE), 3);
 PUSH_DATA (push, 1024);
-   PUSH_DATAh(push, nvc0->screen->uniform_bo->offset + (6 << 16) + (s << 10));
-   PUSH_DATA (push, nvc0->screen->uniform_bo->offset + (6 << 16) + (s << 10));
+   PUSH_DATAh(push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(s));
+   PUSH_DATA (push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(s));
 BEGIN_1IC0(push, NVC0_CP(CB_POS), 1 + 4 * NVC0_MAX_BUFFERS);
-   PUSH_DATA (push, 512);
+   PUSH_DATA (push, NVC0_CB_AUX_BUF_INFO(0));

 for (i = 0; i < NVC0_MAX_BUFFERS; i++) {
if (nvc0->buffers[s][i].buffer) {
diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h 
b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
index 54afe88..c63d138 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
@@ -98,6 +98,28 @@
  #define NVC0_BIND_M2MF  0
  #define NVC0_BIND_FENCE 1

+/* 6 user uniform buffers, at 64K each */
+#define NVC0_CB_USR_INFO(s) (s << 16)
+#define NVC0_CB_USR_SIZE(6 << 16)
+/* 6 driver constbuts, at 1K each */
+#define NVC0_CB_AUX_INFO(s) NVC0_CB_USR_SIZE + (s << 10)
+#define NVC0_CB_AUX_SIZE(6 << 10)
+/* 32 textures handles, at 1 32-bits integer each */
+#define NVC0_CB_AUX_TEX_INFO(i) 0x020 + (i) * 4
+#define NVC0_CB_AUX_TEX_SIZE(32 * 4)
+/* 8 user clip planes, at 4 32-bits floats each */
+#define NVC0_CB_AUX_UCP_INFO0x100


This one (and the following ones) seems to be placed in memory one after the
other. Wouldn't it be better to define them relative to each other, rather than
hardcoding the value. It blurs how they are layout in memory, and makes it more
prone to errors if one decides to insert something in between two, or do some
other modifications.


+#define NVC0_CB_AUX_UCP_SIZE(PIPE_MAX_CLIP_PLANES * 4 * 4)
+/* 8 sets of 32-bits integer pairs sample offsets */
+#define NVC0_CB_AUX_SAMPLE_INFO 0x180 /* FP */
+#define NVC0_CB_AUX_SAMPLE_SIZE (8 * 4 * 2)
+/* draw parameters (index bais, base instance, drawid) */
+#define NVC0_CB_AUX_DRAW_INFO   0x180 /* VP */


What is the status of this one? Is the region from 0x180 and after considered
as the `AUX_DRAW_INFO`, and has a subpart named as `AUX_SAMPLE_INFO`, or

Re: [Mesa-dev] [PATCH 4/5] nvc0: avoid using magic numbers for the uniform_bo offsets

2016-03-18 Thread Pierre Moreau 
Hello,

On 09:55 PM - Mar 15 2016, Samuel Pitoiset wrote:
> Instead make use of constants to improve readability.
> 
> Signed-off-by: Samuel Pitoiset 
> ---
>  src/gallium/drivers/nouveau/nvc0/nvc0_compute.c| 13 +-
>  src/gallium/drivers/nouveau/nvc0/nvc0_context.h| 22 +
>  src/gallium/drivers/nouveau/nvc0/nvc0_program.c| 12 +-
>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 12 +-
>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.h |  2 +-
>  .../drivers/nouveau/nvc0/nvc0_state_validate.c | 28 
> --
>  src/gallium/drivers/nouveau/nvc0/nvc0_tex.c|  9 ---
>  src/gallium/drivers/nouveau/nvc0/nvc0_vbo.c| 14 ++-
>  8 files changed, 69 insertions(+), 43 deletions(-)
> 
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c 
> b/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
> index ffbb16f..6aaa7ce 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
> @@ -153,7 +153,7 @@ nvc0_compute_validate_constbufs(struct nvc0_context *nvc0)
>  
>if (nvc0->constbuf[s][i].user) {
>   struct nouveau_bo *bo = nvc0->screen->uniform_bo;
> - const unsigned base = s << 16;
> + const unsigned base = NVC0_CB_USR_INFO(s);
>   const unsigned size = nvc0->constbuf[s][0].size;
>   assert(i == 0); /* we really only want OpenGL uniforms here */
>   assert(nvc0->constbuf[s][0].u.data);
> @@ -207,8 +207,8 @@ nvc0_compute_validate_driverconst(struct nvc0_context 
> *nvc0)
>  
> BEGIN_NVC0(push, NVC0_CP(CB_SIZE), 3);
> PUSH_DATA (push, 1024);
> -   PUSH_DATAh(push, screen->uniform_bo->offset + (6 << 16) + (5 << 10));
> -   PUSH_DATA (push, screen->uniform_bo->offset + (6 << 16) + (5 << 10));
> +   PUSH_DATAh(push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(5));
> +   PUSH_DATA (push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(5));
> BEGIN_NVC0(push, NVC0_CP(CB_BIND), 1);
> PUSH_DATA (push, (15 << 8) | 1);
>  
> @@ -219,15 +219,16 @@ static void
>  nvc0_compute_validate_buffers(struct nvc0_context *nvc0)
>  {
> struct nouveau_pushbuf *push = nvc0->base.pushbuf;
> +   struct nvc0_screen *screen = nvc0->screen;

Why define `screen` and not `offset`? You would *save* more characters. It's
not like you are reusing screen otherwise in this function. Sure the same
pattern comes back multiple times in various functions, but you should be able
to do the same factorisation in those as well.

> const int s = 5;
> int i;
>  
> BEGIN_NVC0(push, NVC0_CP(CB_SIZE), 3);
> PUSH_DATA (push, 1024);
> -   PUSH_DATAh(push, nvc0->screen->uniform_bo->offset + (6 << 16) + (s << 
> 10));
> -   PUSH_DATA (push, nvc0->screen->uniform_bo->offset + (6 << 16) + (s << 
> 10));
> +   PUSH_DATAh(push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(s));
> +   PUSH_DATA (push, screen->uniform_bo->offset + NVC0_CB_AUX_INFO(s));
> BEGIN_1IC0(push, NVC0_CP(CB_POS), 1 + 4 * NVC0_MAX_BUFFERS);
> -   PUSH_DATA (push, 512);
> +   PUSH_DATA (push, NVC0_CB_AUX_BUF_INFO(0));
>  
> for (i = 0; i < NVC0_MAX_BUFFERS; i++) {
>if (nvc0->buffers[s][i].buffer) {
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h 
> b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
> index 54afe88..c63d138 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
> @@ -98,6 +98,28 @@
>  #define NVC0_BIND_M2MF  0
>  #define NVC0_BIND_FENCE 1
>  
> +/* 6 user uniform buffers, at 64K each */
> +#define NVC0_CB_USR_INFO(s) (s << 16)
> +#define NVC0_CB_USR_SIZE(6 << 16)
> +/* 6 driver constbuts, at 1K each */
> +#define NVC0_CB_AUX_INFO(s) NVC0_CB_USR_SIZE + (s << 10)
> +#define NVC0_CB_AUX_SIZE(6 << 10)
> +/* 32 textures handles, at 1 32-bits integer each */
> +#define NVC0_CB_AUX_TEX_INFO(i) 0x020 + (i) * 4
> +#define NVC0_CB_AUX_TEX_SIZE(32 * 4)
> +/* 8 user clip planes, at 4 32-bits floats each */
> +#define NVC0_CB_AUX_UCP_INFO0x100

This one (and the following ones) seems to be placed in memory one after the
other. Wouldn't it be better to define them relative to each other, rather than
hardcoding the value. It blurs how they are layout in memory, and makes it more
prone to errors if one decides to insert something in between two, or do some
other modifications.

> +#define NVC0_CB_AUX_UCP_SIZE(PIPE_MAX_CLIP_PLANES * 4 * 4)
> +/* 8 sets of 32-bits integer pairs sample offsets */
> +#define NVC0_CB_AUX_SAMPLE_INFO 0x180 /* FP */
> +#define NVC0_CB_AUX_SAMPLE_SIZE (8 * 4 * 2)
> +/* draw parameters (index bais, base instance, drawid) */
> +#define NVC0_CB_AUX_DRAW_INFO   0x180 /* VP */

What is the status of this one? Is the region from 0x180 and after considered
as the `AUX_DRAW_INFO`, and has a subpart named as