i965: Simplify MOCS mashing in genX_state_upload.c.

Submitted by Kenneth Graunke on Aug. 23, 2017, 10:57 p.m.

Details

Message ID 20170823225737.11304-1-kenneth@whitecape.org
State New
Headers show
Series "i965: Simplify MOCS mashing in genX_state_upload.c." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Aug. 23, 2017, 10:57 p.m.
Instead of having a proliferation of generation checks and MOCS values,
we can just #define MOCS_ALL to the generation-specific value for "use
as many caches as possible" and use that in various places.

This should make it easier to change MOCS values, as there are fewer
places that need updating.
---
 src/mesa/drivers/dri/i965/genX_state_upload.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
index f1e9fa38ffc..f2bbe4e9897 100644
--- a/src/mesa/drivers/dri/i965/genX_state_upload.c
+++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
@@ -143,6 +143,16 @@  KSP(struct brw_context *brw, uint32_t offset)
 }
 #endif
 
+#if GEN_GEN == 10
+#define MOCS_ALL CNL_MOCS_WB
+#elif GEN_GEN == 9
+#define MOCS_ALL SKL_MOCS_WB
+#elif GEN_GEN == 8
+#define MOCS_ALL BDW_MOCS_WB
+#elif GEN_GEN == 7
+#define MOCS_ALL GEN7_MOCS_L3
+#endif
+
 #include "genxml/genX_pack.h"
 
 #define _brw_cmd_length(cmd) cmd ## _length
@@ -323,6 +333,7 @@  genX(emit_vertex_buffer_state)(struct brw_context *brw,
 
 #if GEN_GEN >= 7
       .AddressModifyEnable = true,
+      .VertexBufferMOCS = MOCS_ALL,
 #endif
 
 #if GEN_GEN < 8
@@ -331,16 +342,6 @@  genX(emit_vertex_buffer_state)(struct brw_context *brw,
 #if GEN_GEN >= 5
       .EndAddress = ro_bo(bo, end_offset - 1),
 #endif
-#endif
-
-#if GEN_GEN == 10
-      .VertexBufferMOCS = CNL_MOCS_WB,
-#elif GEN_GEN == 9
-      .VertexBufferMOCS = SKL_MOCS_WB,
-#elif GEN_GEN == 8
-      .VertexBufferMOCS = BDW_MOCS_WB,
-#elif GEN_GEN == 7
-      .VertexBufferMOCS = GEN7_MOCS_L3,
 #endif
    };
 
@@ -847,7 +848,7 @@  genX(emit_index_buffer)(struct brw_context *brw)
       ib.IndexFormat = brw_get_index_type(index_buffer->index_size);
       ib.BufferStartingAddress = ro_bo(brw->ib.bo, 0);
 #if GEN_GEN >= 8
-      ib.IndexBufferMOCS = GEN_GEN >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
+      ib.IndexBufferMOCS = MOCS_ALL;
       ib.BufferSize = brw->ib.size;
 #else
       ib.BufferEndingAddress = ro_bo(brw->ib.bo, brw->ib.size - 1);
@@ -3599,7 +3600,6 @@  genX(upload_3dstate_so_buffers)(struct brw_context *brw)
 #else
    struct brw_transform_feedback_object *brw_obj =
       (struct brw_transform_feedback_object *) xfb_obj;
-   uint32_t mocs_wb = GEN_GEN >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
 #endif
 
    /* Set up the up to 4 output buffers.  These are the ranges defined in the
@@ -3634,7 +3634,7 @@  genX(upload_3dstate_so_buffers)(struct brw_context *brw)
          sob.SOBufferEnable = true;
          sob.StreamOffsetWriteEnable = true;
          sob.StreamOutputBufferOffsetAddressEnable = true;
-         sob.SOBufferMOCS = mocs_wb;
+         sob.SOBufferMOCS = MOCS_ALL;
 
          sob.SurfaceSize = MAX2(xfb_obj->Size[i] / 4, 1) - 1;
          sob.StreamOutputBufferOffsetAddress =

Comments

Looks good, but it looks like you could replace an additional one in 
upload_push_constant_packets().
Also why not name it GEN_MOCS ? (so it's a bit more consistent with 
other macros defined per gen).

Thanks!

On 23/08/17 23:57, Kenneth Graunke wrote:
> Instead of having a proliferation of generation checks and MOCS values,
> we can just #define MOCS_ALL to the generation-specific value for "use
> as many caches as possible" and use that in various places.
>
> This should make it easier to change MOCS values, as there are fewer
> places that need updating.
> ---
>   src/mesa/drivers/dri/i965/genX_state_upload.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
> index f1e9fa38ffc..f2bbe4e9897 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -143,6 +143,16 @@ KSP(struct brw_context *brw, uint32_t offset)
>   }
>   #endif
>   
> +#if GEN_GEN == 10
> +#define MOCS_ALL CNL_MOCS_WB
> +#elif GEN_GEN == 9
> +#define MOCS_ALL SKL_MOCS_WB
> +#elif GEN_GEN == 8
> +#define MOCS_ALL BDW_MOCS_WB
> +#elif GEN_GEN == 7
> +#define MOCS_ALL GEN7_MOCS_L3
> +#endif
> +
>   #include "genxml/genX_pack.h"
>   
>   #define _brw_cmd_length(cmd) cmd ## _length
> @@ -323,6 +333,7 @@ genX(emit_vertex_buffer_state)(struct brw_context *brw,
>   
>   #if GEN_GEN >= 7
>         .AddressModifyEnable = true,
> +      .VertexBufferMOCS = MOCS_ALL,
>   #endif
>   
>   #if GEN_GEN < 8
> @@ -331,16 +342,6 @@ genX(emit_vertex_buffer_state)(struct brw_context *brw,
>   #if GEN_GEN >= 5
>         .EndAddress = ro_bo(bo, end_offset - 1),
>   #endif
> -#endif
> -
> -#if GEN_GEN == 10
> -      .VertexBufferMOCS = CNL_MOCS_WB,
> -#elif GEN_GEN == 9
> -      .VertexBufferMOCS = SKL_MOCS_WB,
> -#elif GEN_GEN == 8
> -      .VertexBufferMOCS = BDW_MOCS_WB,
> -#elif GEN_GEN == 7
> -      .VertexBufferMOCS = GEN7_MOCS_L3,
>   #endif
>      };
>   
> @@ -847,7 +848,7 @@ genX(emit_index_buffer)(struct brw_context *brw)
>         ib.IndexFormat = brw_get_index_type(index_buffer->index_size);
>         ib.BufferStartingAddress = ro_bo(brw->ib.bo, 0);
>   #if GEN_GEN >= 8
> -      ib.IndexBufferMOCS = GEN_GEN >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
> +      ib.IndexBufferMOCS = MOCS_ALL;
>         ib.BufferSize = brw->ib.size;
>   #else
>         ib.BufferEndingAddress = ro_bo(brw->ib.bo, brw->ib.size - 1);
> @@ -3599,7 +3600,6 @@ genX(upload_3dstate_so_buffers)(struct brw_context *brw)
>   #else
>      struct brw_transform_feedback_object *brw_obj =
>         (struct brw_transform_feedback_object *) xfb_obj;
> -   uint32_t mocs_wb = GEN_GEN >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
>   #endif
>   
>      /* Set up the up to 4 output buffers.  These are the ranges defined in the
> @@ -3634,7 +3634,7 @@ genX(upload_3dstate_so_buffers)(struct brw_context *brw)
>            sob.SOBufferEnable = true;
>            sob.StreamOffsetWriteEnable = true;
>            sob.StreamOutputBufferOffsetAddressEnable = true;
> -         sob.SOBufferMOCS = mocs_wb;
> +         sob.SOBufferMOCS = MOCS_ALL;
>   
>            sob.SurfaceSize = MAX2(xfb_obj->Size[i] / 4, 1) - 1;
>            sob.StreamOutputBufferOffsetAddress =
On Thursday, August 24, 2017 4:04:26 AM PDT Lionel Landwerlin wrote:
> Looks good, but it looks like you could replace an additional one in 
> upload_push_constant_packets().

That one is a bit weird - it uses 0 on Gen8+.  I've wondered about that,
actually - the docs claim that you must use 0 - but at least on Skylake,
0 is an entry in the table that means uncached.  So is the requirement
that the bits be 0, or the requirement that you bypass caching?

Things we'll never know I guess.  I'm not sure if it matters, though,
since it's just pulling the data into a segment of the L3 anyway...so
it's only read one time...

At any rate, I left it open coded because it's different than the others.

> Also why not name it GEN_MOCS ? (so it's a bit more consistent with 
> other macros defined per gen).
> 
> Thanks!

I like this.  Changed locally.
On 24/08/17 16:15, Kenneth Graunke wrote:
> On Thursday, August 24, 2017 4:04:26 AM PDT Lionel Landwerlin wrote:
>> Looks good, but it looks like you could replace an additional one in
>> upload_push_constant_packets().
> That one is a bit weird - it uses 0 on Gen8+.  I've wondered about that,
> actually - the docs claim that you must use 0 - but at least on Skylake,
> 0 is an entry in the table that means uncached.  So is the requirement
> that the bits be 0, or the requirement that you bypass caching?
>
> Things we'll never know I guess.  I'm not sure if it matters, though,
> since it's just pulling the data into a segment of the L3 anyway...so
> it's only read one time...
>
> At any rate, I left it open coded because it's different than the others.
>
>> Also why not name it GEN_MOCS ? (so it's a bit more consistent with
>> other macros defined per gen).
>>
>> Thanks!
> I like this.  Changed locally.


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>