Re: [Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field
On Wednesday, December 12, 2018 9:17:38 AM PST Jason Ekstrand wrote: > On Wed, Dec 12, 2018 at 11:09 AM Jason Ekstrand > wrote: > > > On Tue, Dec 11, 2018 at 10:31 PM Kenneth Graunke > > wrote: > > > >> When we first started using genxml, we decided to represent MOCS as an > >> actual structure, and pack values. However, in many places, it was more > >> convenient to use a numeric value rather than treating it as a struct, > >> so we added secondary setters in a bunch of places as well. > >> > >> We were not entirely consistent, either. Some places only had one. > >> Gen6 had both kinds of setters for STATE_BASE_ADDRESS, but newer gens > >> only had the struct-based setters. The names were sometimes "Constant > >> Buffer Object Control State" instead of "Memory", making it harder to > >> find. Many had prefixes like "Vertex Buffer MOCS"...in a vertex buffer > >> packet...which is a bit redundant. > >> > >> On modern hardware, MOCS is simply an index into a table, but we were > >> still carrying around the structure with an "Index to MOCS Table" field, > >> in addition to the direct numeric setters. This is clunky - we really > >> just want a number on new hardware. > >> > > > > This gets a bit sticky because the "Index to MOCS Table" field starts at > > bit 1 not 0 so the MOCS value in your patch isn't actually the index, it's > > index * 2. Do we want to "fix" this and make the MOCS value the thing we > > actually want on gen9+? > > > > One other comment: While the MEMORY_OBJECT_CONTROL_STATE field isn't set > by drivers it is decoded by aubinator (though sometimes not correctly :-/) > and may be useful to keep around for that reason. > > Note: Neither of those is a NAK. :-) On Gen6-8, that may be true. But, I always found the MOCS printouts to be confusing and mess up the flow of the decode: 0xe60001f0: 0x78060003: 3DSTATE_STENCIL_BUFFER 0xe60001f0: 0x78060003 : Dword 0 DWord Length: 3 0xe60001f4: 0x : Dword 1 Surface Pitch: 0 Stencil Buffer MOCS: 0 Stencil Buffer Object Control State: 0xe60001f4: 0x : Dword 0 Index to MOCS Tables: 0 Stencil Buffer Enable: false Suddenly, we're in Dword 0...of a field in Dword 1 of the packet? And we print one field...but then we're right back to printing the parent structure fields. (I guess we could print substructures better.) In the meantime, the new output does flow better: 0xe60001f0: 0x78060003: 3DSTATE_STENCIL_BUFFER 0xe60001f0: 0x78060003 : Dword 0 DWord Length: 3 0xe60001f4: 0x : Dword 1 Surface Pitch: 0 MOCS: 0 Stencil Buffer Enable: false 0xe60001f8: 0x : Dword 2 0xe60001fc: 0x : Dword 3 Surface Base Address: 0x 0xe6000200: 0x : Dword 4 Surface QPitch: 0 Reading STATE_BASE_ADDRESS was especially bad, and now it's mostly OK. Of course, you can still compare the numeric value against MOCS defines in the codebase to tell which one it's using. I suspect what we really want for nice decoding is not a structure, or a uint, but a genxml enum value. We'd just have MOCS_WB, MOCS_PTE, MOCS_L3, or whatever other few options we actually use, and print that. --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field
On Wednesday, December 12, 2018 9:09:54 AM PST Jason Ekstrand wrote: > On Tue, Dec 11, 2018 at 10:31 PM Kenneth Graunke > wrote: > > > When we first started using genxml, we decided to represent MOCS as an > > actual structure, and pack values. However, in many places, it was more > > convenient to use a numeric value rather than treating it as a struct, > > so we added secondary setters in a bunch of places as well. > > > > We were not entirely consistent, either. Some places only had one. > > Gen6 had both kinds of setters for STATE_BASE_ADDRESS, but newer gens > > only had the struct-based setters. The names were sometimes "Constant > > Buffer Object Control State" instead of "Memory", making it harder to > > find. Many had prefixes like "Vertex Buffer MOCS"...in a vertex buffer > > packet...which is a bit redundant. > > > > On modern hardware, MOCS is simply an index into a table, but we were > > still carrying around the structure with an "Index to MOCS Table" field, > > in addition to the direct numeric setters. This is clunky - we really > > just want a number on new hardware. > > > > This gets a bit sticky because the "Index to MOCS Table" field starts at > bit 1 not 0 so the MOCS value in your patch isn't actually the index, it's > index * 2. Do we want to "fix" this and make the MOCS value the thing we > actually want on gen9+? > > --Jason That's a good point. It's not great either way, but... Adjusting the MOCS fields in the XML by 1-bit could be confusing when reading the XML. But, the decoder would see the correct index, rather than 2x the index. So, that does seem better? I'll probably go with this for now but I'd be happy to shift things by a bit and fix the numeric values to be real indices on Gen9+. --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field
On Wed, Dec 12, 2018 at 11:09 AM Jason Ekstrand wrote: > On Tue, Dec 11, 2018 at 10:31 PM Kenneth Graunke > wrote: > >> When we first started using genxml, we decided to represent MOCS as an >> actual structure, and pack values. However, in many places, it was more >> convenient to use a numeric value rather than treating it as a struct, >> so we added secondary setters in a bunch of places as well. >> >> We were not entirely consistent, either. Some places only had one. >> Gen6 had both kinds of setters for STATE_BASE_ADDRESS, but newer gens >> only had the struct-based setters. The names were sometimes "Constant >> Buffer Object Control State" instead of "Memory", making it harder to >> find. Many had prefixes like "Vertex Buffer MOCS"...in a vertex buffer >> packet...which is a bit redundant. >> >> On modern hardware, MOCS is simply an index into a table, but we were >> still carrying around the structure with an "Index to MOCS Table" field, >> in addition to the direct numeric setters. This is clunky - we really >> just want a number on new hardware. >> > > This gets a bit sticky because the "Index to MOCS Table" field starts at > bit 1 not 0 so the MOCS value in your patch isn't actually the index, it's > index * 2. Do we want to "fix" this and make the MOCS value the thing we > actually want on gen9+? > One other comment: While the MEMORY_OBJECT_CONTROL_STATE field isn't set by drivers it is decoded by aubinator (though sometimes not correctly :-/) and may be useful to keep around for that reason. Note: Neither of those is a NAK. :-) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field
On Tue, Dec 11, 2018 at 10:31 PM Kenneth Graunke wrote: > When we first started using genxml, we decided to represent MOCS as an > actual structure, and pack values. However, in many places, it was more > convenient to use a numeric value rather than treating it as a struct, > so we added secondary setters in a bunch of places as well. > > We were not entirely consistent, either. Some places only had one. > Gen6 had both kinds of setters for STATE_BASE_ADDRESS, but newer gens > only had the struct-based setters. The names were sometimes "Constant > Buffer Object Control State" instead of "Memory", making it harder to > find. Many had prefixes like "Vertex Buffer MOCS"...in a vertex buffer > packet...which is a bit redundant. > > On modern hardware, MOCS is simply an index into a table, but we were > still carrying around the structure with an "Index to MOCS Table" field, > in addition to the direct numeric setters. This is clunky - we really > just want a number on new hardware. > This gets a bit sticky because the "Index to MOCS Table" field starts at bit 1 not 0 so the MOCS value in your patch isn't actually the index, it's index * 2. Do we want to "fix" this and make the MOCS value the thing we actually want on gen9+? --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field
Reviewed-by: Kristian H. Kristensen On Tue, Dec 11, 2018 at 9:25 PM Jordan Justen wrote: > > Reviewed-by: Jordan Justen > > On 2018-12-11 20:23:06, Kenneth Graunke wrote: > > When we first started using genxml, we decided to represent MOCS as an > > actual structure, and pack values. However, in many places, it was more > > convenient to use a numeric value rather than treating it as a struct, > > so we added secondary setters in a bunch of places as well. > > > > We were not entirely consistent, either. Some places only had one. > > Gen6 had both kinds of setters for STATE_BASE_ADDRESS, but newer gens > > only had the struct-based setters. The names were sometimes "Constant > > Buffer Object Control State" instead of "Memory", making it harder to > > find. Many had prefixes like "Vertex Buffer MOCS"...in a vertex buffer > > packet...which is a bit redundant. > > > > On modern hardware, MOCS is simply an index into a table, but we were > > still carrying around the structure with an "Index to MOCS Table" field, > > in addition to the direct numeric setters. This is clunky - we really > > just want a number on new hardware. > > > > This patch eliminates the struct-based setters, and makes the numeric > > setters be consistently called "MOCS". We leave the struct definition > > around on Gen7-8 for reference purposes, but it is unused. > > --- > > src/intel/blorp/blorp_genX_exec.h | 2 +- > > src/intel/genxml/gen10.xml| 53 + > > src/intel/genxml/gen11.xml| 53 + > > src/intel/genxml/gen6.xml | 28 ++- > > src/intel/genxml/gen7.xml | 35 - > > src/intel/genxml/gen75.xml| 38 -- > > src/intel/genxml/gen8.xml | 47 +--- > > src/intel/genxml/gen9.xml | 50 +--- > > src/intel/isl/isl_emit_depth_stencil.c| 6 +- > > src/intel/vulkan/anv_private.h| 76 --- > > src/intel/vulkan/gen7_cmd_buffer.c| 2 +- > > src/intel/vulkan/gen8_cmd_buffer.c| 2 +- > > src/intel/vulkan/genX_cmd_buffer.c| 19 +++-- > > src/intel/vulkan/genX_gpu_memcpy.c| 4 +- > > src/intel/vulkan/genX_state.c | 6 +- > > src/mesa/drivers/dri/i965/genX_state_upload.c | 14 ++-- > > 16 files changed, 177 insertions(+), 258 deletions(-) > > > > diff --git a/src/intel/blorp/blorp_genX_exec.h > > b/src/intel/blorp/blorp_genX_exec.h > > index 065980616ec..42494ffbc86 100644 > > --- a/src/intel/blorp/blorp_genX_exec.h > > +++ b/src/intel/blorp/blorp_genX_exec.h > > @@ -311,7 +311,7 @@ blorp_fill_vertex_buffer_state(struct blorp_batch > > *batch, > > vb[idx].BufferPitch = stride; > > > > #if GEN_GEN >= 6 > > - vb[idx].VertexBufferMOCS = addr.mocs; > > + vb[idx].MOCS = addr.mocs; > > #endif > > > > #if GEN_GEN >= 7 > > diff --git a/src/intel/genxml/gen10.xml b/src/intel/genxml/gen10.xml > > index 2d3bc39b1b9..21cd8a17d91 100644 > > --- a/src/intel/genxml/gen10.xml > > +++ b/src/intel/genxml/gen10.xml > > @@ -219,14 +219,9 @@ > > > type="uint"/> > > > > > > - > > - > > - > > - > > > > > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > - > > + > > > > > > > > @@ -495,7 +490,6 @@ > > > type="bool"/> > > > type="bool"/> > > > type="bool"/> > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > > > > > > > @@ -993,7 +987,7 @@ > > > > > type="address"/> > > > type="uint"/> > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > + > > > type="uint"> > > > > > > @@ -1085,7 +1079,7 @@ > > > default="3"/> > > > default="0"/> > > > default="26"/> > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > + > > > > > type="3DSTATE_CONSTANT_BODY"/> > > > > @@ -1095,7 +1089,7 @@ > > > default="3"/> > > > default="0"/> > > > default="22"/> > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > + > > > > > type="3DSTATE_CONSTANT_BODY"/> > > > > @@ -1105,7 +1099,7 @@ > > > default="3"/> > > > default="0"/> > > > default="25"/> > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > + > > > > > type="3DSTATE_CONSTANT_BODY"/> > > > > @@ -1116,7 +1110,7 @@ > > > default="0"/> > > > default="23"/> > > > type="uint"/> > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > + > > > > > type="3DSTATE_CONSTANT_BODY"/> > > > > @@ -1126,7 +1120,7 @@ > > > default="3"/> > > > default="0"/> > > > default="21"/> > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > + > > > > > type="3DSTATE_CONSTANT_BODY"/> > > > > @@ -1157,8 +1151,7 @@ > > > > > > > > - > type="MEMORY_OBJECT_CONTROL_STATE"/> > > - > > + > > > >
Re: [Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field
Reviewed-by: Jordan Justen On 2018-12-11 20:23:06, Kenneth Graunke wrote: > When we first started using genxml, we decided to represent MOCS as an > actual structure, and pack values. However, in many places, it was more > convenient to use a numeric value rather than treating it as a struct, > so we added secondary setters in a bunch of places as well. > > We were not entirely consistent, either. Some places only had one. > Gen6 had both kinds of setters for STATE_BASE_ADDRESS, but newer gens > only had the struct-based setters. The names were sometimes "Constant > Buffer Object Control State" instead of "Memory", making it harder to > find. Many had prefixes like "Vertex Buffer MOCS"...in a vertex buffer > packet...which is a bit redundant. > > On modern hardware, MOCS is simply an index into a table, but we were > still carrying around the structure with an "Index to MOCS Table" field, > in addition to the direct numeric setters. This is clunky - we really > just want a number on new hardware. > > This patch eliminates the struct-based setters, and makes the numeric > setters be consistently called "MOCS". We leave the struct definition > around on Gen7-8 for reference purposes, but it is unused. > --- > src/intel/blorp/blorp_genX_exec.h | 2 +- > src/intel/genxml/gen10.xml| 53 + > src/intel/genxml/gen11.xml| 53 + > src/intel/genxml/gen6.xml | 28 ++- > src/intel/genxml/gen7.xml | 35 - > src/intel/genxml/gen75.xml| 38 -- > src/intel/genxml/gen8.xml | 47 +--- > src/intel/genxml/gen9.xml | 50 +--- > src/intel/isl/isl_emit_depth_stencil.c| 6 +- > src/intel/vulkan/anv_private.h| 76 --- > src/intel/vulkan/gen7_cmd_buffer.c| 2 +- > src/intel/vulkan/gen8_cmd_buffer.c| 2 +- > src/intel/vulkan/genX_cmd_buffer.c| 19 +++-- > src/intel/vulkan/genX_gpu_memcpy.c| 4 +- > src/intel/vulkan/genX_state.c | 6 +- > src/mesa/drivers/dri/i965/genX_state_upload.c | 14 ++-- > 16 files changed, 177 insertions(+), 258 deletions(-) > > diff --git a/src/intel/blorp/blorp_genX_exec.h > b/src/intel/blorp/blorp_genX_exec.h > index 065980616ec..42494ffbc86 100644 > --- a/src/intel/blorp/blorp_genX_exec.h > +++ b/src/intel/blorp/blorp_genX_exec.h > @@ -311,7 +311,7 @@ blorp_fill_vertex_buffer_state(struct blorp_batch *batch, > vb[idx].BufferPitch = stride; > > #if GEN_GEN >= 6 > - vb[idx].VertexBufferMOCS = addr.mocs; > + vb[idx].MOCS = addr.mocs; > #endif > > #if GEN_GEN >= 7 > diff --git a/src/intel/genxml/gen10.xml b/src/intel/genxml/gen10.xml > index 2d3bc39b1b9..21cd8a17d91 100644 > --- a/src/intel/genxml/gen10.xml > +++ b/src/intel/genxml/gen10.xml > @@ -219,14 +219,9 @@ > > > > - > - > - > - > > > - type="MEMORY_OBJECT_CONTROL_STATE"/> > - > + > > > > @@ -495,7 +490,6 @@ > type="bool"/> > type="bool"/> > type="bool"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > > > > @@ -993,7 +987,7 @@ > > type="address"/> > > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > type="uint"> > > > @@ -1085,7 +1079,7 @@ > default="3"/> > default="0"/> > default="26"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1095,7 +1089,7 @@ > default="3"/> > default="0"/> > default="22"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1105,7 +1099,7 @@ > default="3"/> > default="0"/> > default="25"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1116,7 +1110,7 @@ > default="0"/> > default="23"/> > type="uint"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1126,7 +1120,7 @@ > default="3"/> > default="0"/> > default="21"/> > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > type="3DSTATE_CONSTANT_BODY"/> > > @@ -1157,8 +1151,7 @@ > > > > - type="MEMORY_OBJECT_CONTROL_STATE"/> > - > + > > > > @@ -1368,7 +1361,7 @@ > > type="address"/> > > - type="MEMORY_OBJECT_CONTROL_STATE"/> > + > > > > @@ -1447,8 +1440,7 @@ > default="0"/> > default="7"/> > > - end="63" type="MEMORY_OBJECT_CONTROL_STATE"/> > - type="uint"/> > + > > > > @@ -1511,8 +1503,7 @@ > > > > - type="MEMORY_OBJECT_CONTROL_STATE"/> > - > + > type="address"/> >
