[Mesa-dev] [PATCH] genxml: Consistently use a numeric "MOCS" field

[Kenneth Graunke]

On Wednesday, December 12, 2018 9:17:38 AM PST Jason Ekstrand wrote:
> On Wed, Dec 12, 2018 at 11:09 AM Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> 
> > On Tue, Dec 11, 2018 at 10:31 PM Kenneth Graunke <kenneth at whitecape.org>
> > 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:

0xffffffffe60001f0:  0x78060003:  3DSTATE_STENCIL_BUFFER                        
                                  
0xffffffffe60001f0:  0x78060003 : Dword 0
    DWord Length: 3
0xffffffffe60001f4:  0x00000000 : Dword 1
    Surface Pitch: 0
    Stencil Buffer MOCS: 0
    Stencil Buffer Object Control State: <struct MEMORY_OBJECT_CONTROL_STATE>
0xffffffffe60001f4:  0x00000000 : 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:

0xffffffffe60001f0:  0x78060003:  3DSTATE_STENCIL_BUFFER                                                          
0xffffffffe60001f0:  0x78060003 : Dword 0
    DWord Length: 3
0xffffffffe60001f4:  0x00000000 : Dword 1
    Surface Pitch: 0
    MOCS: 0
    Stencil Buffer Enable: false
0xffffffffe60001f8:  0x00000000 : Dword 2
0xffffffffe60001fc:  0x00000000 : Dword 3
    Surface Base Address: 0x00000000
0xffffffffe6000200:  0x00000000 : 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181214/bd9d9bf7/attachment.sig>