[Mesa-dev] [PATCH v3 05/13] intel/genxml: Add Clear Color struct.

[Rafael Antognolli]

On Tue, Feb 27, 2018 at 11:46:12AM -0800, Jason Ekstrand wrote:
> On Tue, Feb 27, 2018 at 9:35 AM, Rafael Antognolli <rafael.antognolli at intel.com
> > wrote:
> 
>     On Mon, Feb 26, 2018 at 05:04:37PM -0800, Jason Ekstrand wrote:
>     > On Wed, Feb 21, 2018 at 1:45 PM, Rafael Antognolli <
>     rafael.antognolli at intel.com
>     > > wrote:
>     >
>     >     The size of the clear color struct (expected by the hardware) is 8
>     >     dwords (isl_dev.ss.clear_value_state_size here). But we still need to
>     >     track the size of the clear color, used when memcopying it to/from
>     the
>     >     state buffer. For that we keep isl_dev.ss.clear_value_size.
>     >
>     >     Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
>     >     ---
>     >      src/intel/genxml/gen10.xml     | 8 ++++++++
>     >      src/intel/isl/isl.c            | 4 +++-
>     >      src/intel/isl/isl.h            | 5 +++++
>     >      src/intel/vulkan/anv_image.c   | 2 +-
>     >      src/intel/vulkan/anv_private.h | 2 +-
>     >      5 files changed, 18 insertions(+), 3 deletions(-)
>     >
>     >     diff --git a/src/intel/genxml/gen10.xml b/src/intel/genxml/gen10.xml
>     >     index b434d1b0f66..58b83954c4c 100644
>     >     --- a/src/intel/genxml/gen10.xml
>     >     +++ b/src/intel/genxml/gen10.xml
>     >
>     >
>     > We need gen11 as well
>     >
>     >
>     >     @@ -809,6 +809,14 @@
>     >          <field name="Alpha Clear Color" start="480" end="511" type="int"
>     />
>     >        </struct>
>     >
>     >     +  <struct name="CLEAR_COLOR" length="8">
>     >     +    <field name="Raw Clear Color Red" start="0" end="31" type="int"
>     />
>     >     +    <field name="Raw Clear Color Green" start="32" end="63" type=
>     "int"/>
>     >     +    <field name="Raw Clear Color Blue" start="64" end="95" type=
>     "int"/>
>     >     +    <field name="Raw Clear Color Alpha" start="96" end="127" type=
>     "int"/>
>     >
>     >
>     > Might be good to put Converted Clear Value Hi/Low in here as well.
>     >
>     >
>     >     +    <!-- Reserved - MBZ -->
>     >     +  </struct>
>     >     +
>     >        <struct name="SAMPLER_INDIRECT_STATE_BORDER_COLOR" length="4">
>     >          <field name="Border Color Red As S31" start="0" end="31" type=
>     "int"/>
>     >          <field name="Border Color Red As U32" start="0" end="31" type=
>     "uint"/>
>     >     diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
>     >     index 77641a89f86..e94470362e2 100644
>     >     --- a/src/intel/isl/isl.c
>     >     +++ b/src/intel/isl/isl.c
>     >     @@ -79,9 +79,10 @@ isl_device_init(struct isl_device *dev,
>     >             * - 2 dwords that can be used by the hardware for converted
>     clear
>     >     color
>     >             *   + some extra bits.
>     >             */
>     >     -      dev->ss.clear_value_size = 8 * 4;
>     >     +      dev->ss.clear_value_size = 4 * 4;
>     >            dev->ss.clear_value_offset =
>     >               RENDER_SURFACE_STATE_ClearValueAddress_start(info) / 32 *
>     4;
>     >     +      dev->ss.clear_value_state_size = CLEAR_COLOR_length(info) * 4;
>     >         } else {
>     >            dev->ss.clear_value_size =
>     >               isl_align(RENDER_SURFACE_STATE_RedClearColor_bits(info) +
>     >     @@ -90,6 +91,7 @@ isl_device_init(struct isl_device *dev,
>     >                         RENDER_SURFACE_STATE_AlphaClearColor_bits(info),
>     32) /
>     >     8;
>     >            dev->ss.clear_value_offset =
>     >               RENDER_SURFACE_STATE_RedClearColor_start(info) / 32 * 4;
>     >     +      dev->ss.clear_value_state_size = dev->ss.clear_value_size;
>     >
>     >
>     > Ugh... Let's just make these two separate things.  clear_value_size will
>     be 4 *
>     > 4 on gen9-10 and clear_color_state_size will be 8*4 on gen10+
> 
>     I'm not sure I understand/agree with you here. clear_value_size should
>     be 4 * 4 everywhere, since we use this to memcpy the 4 dwords of the
>     clear color. So, are you suggesting we remove clear_value_state_size
>     from gen9?
> 
> 
> I mean that we have two separate things here: clear_value_size which is 4 B on
> gen7-8, 16 B on gen9-10, and doesn't exist on gen11+ and clear_color_state_size
> which doesn't exist prior to gen10 and is 32 B on gen10+.  Does that make more
> sense?

Hmm... I was planning to use clear_value_size on gen11+ as well. If I'm
not wrong, there are some loops where we cycle through the dwords in the
clear color state buffer using clear_value_size as a limit, but we can
simply drop it and use 4 (dwords). But yeah, I can drop it...

I agree with clear_color_state_size, though.

>     > I think this means dropping the previous patch entirely and just making
>     this
>     > patch add a size field.
> 
>     Ack.
>    
>     >         }
>     >         assert(RENDER_SURFACE_STATE_SurfaceBaseAddress_start(info) % 8 ==
>     0);
>     >         dev->ss.addr_offset =