[Mesa-dev] i965/gen8/cs: fix constant push buffer

Submitted by Iago Toral Quiroga on Dec. 15, 2015, 11:55 a.m.

Details

Message ID 1450180515-20306-1-git-send-email-itoral@igalia.com
State Superseded
Headers show
Series "i965/gen8/cs: fix constant push buffer" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Dec. 15, 2015, 11:55 a.m.
Page 502 of the Command Reference Broadwell PRM says that CURBE Total
Data Length must be 64-bit aligned.

Fixes the following CTS tests:
ES31-CTS.shader_storage_buffer_object.basic-atomic-case1-cs
ES31-CTS.shader_storage_buffer_object.basic-operations-case1-cs
ES31-CTS.shader_storage_buffer_object.basic-operations-case2-cs
ES31-CTS.shader_storage_buffer_object.basic-stdLayout_UBO_SSBO-case2-cs
ES31-CTS.shader_storage_buffer_object.advanced-write-fragment-cs
ES31-CTS.shader_storage_buffer_object.advanced-indirectAddressing-case2-cs
ES31-CTS.shader_storage_buffer_object.advanced-matrix-cs
---
 src/mesa/drivers/dri/i965/gen7_cs_state.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/gen7_cs_state.c b/src/mesa/drivers/dri/i965/gen7_cs_state.c
index 1fde69c..dbd1967 100644
--- a/src/mesa/drivers/dri/i965/gen7_cs_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_cs_state.c
@@ -77,7 +77,8 @@  brw_upload_cs_state(struct brw_context *brw)
 
    unsigned push_constant_data_size =
       (prog_data->nr_params + local_id_dwords) * sizeof(gl_constant_value);
-   unsigned reg_aligned_constant_size = ALIGN(push_constant_data_size, 32);
+   unsigned reg_aligned_constant_size =
+      ALIGN(push_constant_data_size, brw->gen < 8 ? 32 : 64);
    unsigned push_constant_regs = reg_aligned_constant_size / 32;
    unsigned threads = get_cs_thread_count(cs_prog_data);
 
@@ -241,7 +242,8 @@  brw_upload_cs_push_constants(struct brw_context *brw,
 
       const unsigned push_constant_data_size =
          (local_id_dwords + prog_data->nr_params) * sizeof(gl_constant_value);
-      const unsigned reg_aligned_constant_size = ALIGN(push_constant_data_size, 32);
+      const unsigned reg_aligned_constant_size =
+         ALIGN(push_constant_data_size, brw->gen < 8 ? 32 : 64);
       const unsigned param_aligned_count =
          reg_aligned_constant_size / sizeof(*param);
 

Comments

Thanks Iago!

This patch does not only fix the ssbo test mentioned below, but a lot of other GLES 3.1 CTS tests.

> -----Original Message-----
> From: Iago Toral Quiroga [mailto:itoral@igalia.com]
> Sent: Tuesday, December 15, 2015 12:55 PM
> To: mesa-dev@lists.freedesktop.org
> Cc: Lofstedt, Marta; Justen, Jordan L; Palli, Tapani; Iago Toral Quiroga
> Subject: [PATCH] i965/gen8/cs: fix constant push buffer
> 
> Page 502 of the Command Reference Broadwell PRM says that CURBE Total
> Data Length must be 64-bit aligned.
> 
> Fixes the following CTS tests:
> ES31-CTS.shader_storage_buffer_object.basic-atomic-case1-cs
> ES31-CTS.shader_storage_buffer_object.basic-operations-case1-cs
> ES31-CTS.shader_storage_buffer_object.basic-operations-case2-cs
> ES31-CTS.shader_storage_buffer_object.basic-stdLayout_UBO_SSBO-case2-
> cs
> ES31-CTS.shader_storage_buffer_object.advanced-write-fragment-cs
> ES31-CTS.shader_storage_buffer_object.advanced-indirectAddressing-
> case2-cs
> ES31-CTS.shader_storage_buffer_object.advanced-matrix-cs
> ---
>  src/mesa/drivers/dri/i965/gen7_cs_state.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen7_cs_state.c
> b/src/mesa/drivers/dri/i965/gen7_cs_state.c
> index 1fde69c..dbd1967 100644
> --- a/src/mesa/drivers/dri/i965/gen7_cs_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_cs_state.c
> @@ -77,7 +77,8 @@ brw_upload_cs_state(struct brw_context *brw)
> 
>     unsigned push_constant_data_size =
>        (prog_data->nr_params + local_id_dwords) * sizeof(gl_constant_value);
> -   unsigned reg_aligned_constant_size = ALIGN(push_constant_data_size,
> 32);
> +   unsigned reg_aligned_constant_size =
> +      ALIGN(push_constant_data_size, brw->gen < 8 ? 32 : 64);
>     unsigned push_constant_regs = reg_aligned_constant_size / 32;
>     unsigned threads = get_cs_thread_count(cs_prog_data);
> 
> @@ -241,7 +242,8 @@ brw_upload_cs_push_constants(struct brw_context
> *brw,
> 
>        const unsigned push_constant_data_size =
>           (local_id_dwords + prog_data->nr_params) *
> sizeof(gl_constant_value);
> -      const unsigned reg_aligned_constant_size =
> ALIGN(push_constant_data_size, 32);
> +      const unsigned reg_aligned_constant_size =
> +         ALIGN(push_constant_data_size, brw->gen < 8 ? 32 : 64);
>        const unsigned param_aligned_count =
>           reg_aligned_constant_size / sizeof(*param);
> 
> --
> 1.9.1
Ah! I had just also discovered this issue yesterday in some related
work but I didn't get the chance to try the CTS yet! :)

For the subject I had: "Gen 8 requires 64 byte alignment for push
constant data"

On 2015-12-15 03:55:15, Iago Toral Quiroga wrote:
> Page 502 of the Command Reference Broadwell PRM says that CURBE Total
> Data Length must be 64-bit aligned.

I think both the base and the size alignments are bumped from 32 to
64. Could you add the base address? How about giving the
volume/chapter/section in the spec reference rather than the page
number?

Also, could you update the call to brw_state_batch to also use 64 byte
alignment for the base on gen8+?

-Jordan

> 
> Fixes the following CTS tests:
> ES31-CTS.shader_storage_buffer_object.basic-atomic-case1-cs
> ES31-CTS.shader_storage_buffer_object.basic-operations-case1-cs
> ES31-CTS.shader_storage_buffer_object.basic-operations-case2-cs
> ES31-CTS.shader_storage_buffer_object.basic-stdLayout_UBO_SSBO-case2-cs
> ES31-CTS.shader_storage_buffer_object.advanced-write-fragment-cs
> ES31-CTS.shader_storage_buffer_object.advanced-indirectAddressing-case2-cs
> ES31-CTS.shader_storage_buffer_object.advanced-matrix-cs
> ---
>  src/mesa/drivers/dri/i965/gen7_cs_state.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen7_cs_state.c b/src/mesa/drivers/dri/i965/gen7_cs_state.c
> index 1fde69c..dbd1967 100644
> --- a/src/mesa/drivers/dri/i965/gen7_cs_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_cs_state.c
> @@ -77,7 +77,8 @@ brw_upload_cs_state(struct brw_context *brw)
>  
>     unsigned push_constant_data_size =
>        (prog_data->nr_params + local_id_dwords) * sizeof(gl_constant_value);
> -   unsigned reg_aligned_constant_size = ALIGN(push_constant_data_size, 32);
> +   unsigned reg_aligned_constant_size =
> +      ALIGN(push_constant_data_size, brw->gen < 8 ? 32 : 64);
>     unsigned push_constant_regs = reg_aligned_constant_size / 32;
>     unsigned threads = get_cs_thread_count(cs_prog_data);
>  
> @@ -241,7 +242,8 @@ brw_upload_cs_push_constants(struct brw_context *brw,
>  
>        const unsigned push_constant_data_size =
>           (local_id_dwords + prog_data->nr_params) * sizeof(gl_constant_value);
> -      const unsigned reg_aligned_constant_size = ALIGN(push_constant_data_size, 32);
> +      const unsigned reg_aligned_constant_size =
> +         ALIGN(push_constant_data_size, brw->gen < 8 ? 32 : 64);
>        const unsigned param_aligned_count =
>           reg_aligned_constant_size / sizeof(*param);
>  
> -- 
> 1.9.1
>