[v3,05/13] intel/genxml: Add Clear Color struct.

Submitted by Rafael Antognolli on Feb. 21, 2018, 9:45 p.m.

Details

Message ID 20180221214522.25065-6-rafael.antognolli@intel.com
State New
Headers show
Series "Use clear color address in surface state." ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli Feb. 21, 2018, 9:45 p.m.
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@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(-)

Patch hide | download patch | download mbox

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
@@ -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"/>
+    <!-- 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;
    }
    assert(RENDER_SURFACE_STATE_SurfaceBaseAddress_start(info) % 8 == 0);
    dev->ss.addr_offset =
diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
index 209769a9a99..f1b38efed44 100644
--- a/src/intel/isl/isl.h
+++ b/src/intel/isl/isl.h
@@ -963,6 +963,11 @@  struct isl_device {
       uint8_t aux_addr_offset;
 
       /* Rounded up to the nearest dword to simplify GPU memcpy operations. */
+
+      /* size of the state buffer used to store the clear value + extra
+       * additional space used by the hardware */
+      uint8_t clear_value_state_size;
+      /* size of the clear color itself - used to copy it to/from a BO */
       uint8_t clear_value_size;
       uint8_t clear_value_offset;
    } ss;
diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
index a0aee43bd21..0dafe03442d 100644
--- a/src/intel/vulkan/anv_image.c
+++ b/src/intel/vulkan/anv_image.c
@@ -264,7 +264,7 @@  add_aux_state_tracking_buffer(struct anv_image *image,
    }
 
    /* Clear color and fast clear type */
-   unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
+   unsigned state_size = device->isl_dev.ss.clear_value_state_size + 4;
 
    /* We only need to track compression on CCS_E surfaces. */
    if (image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E) {
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 37e63f56aa0..b8c381d2665 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -2566,7 +2566,7 @@  anv_image_get_fast_clear_type_addr(const struct anv_device *device,
 {
    struct anv_address addr =
       anv_image_get_clear_color_addr(device, image, aspect);
-   addr.offset += device->isl_dev.ss.clear_value_size;
+   addr.offset += device->isl_dev.ss.clear_value_state_size;
    return addr;
 }
 

Comments

On 2018-02-21 13:45:14, Rafael Antognolli 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@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
> @@ -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"/>
> +    <!-- Reserved - MBZ -->

I'm not sure about this comment. Maybe:

    <!-- DW4-7 required by hardware, but not used by software -->

I was going to ask about gen11.xml, but since isl.c is not compiled
per gen, I'm not quite sure how things work with structs in the
genxml.

-Jordan

> +  </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;
>     }
>     assert(RENDER_SURFACE_STATE_SurfaceBaseAddress_start(info) % 8 == 0);
>     dev->ss.addr_offset =
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index 209769a9a99..f1b38efed44 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -963,6 +963,11 @@ struct isl_device {
>        uint8_t aux_addr_offset;
>  
>        /* Rounded up to the nearest dword to simplify GPU memcpy operations. */
> +
> +      /* size of the state buffer used to store the clear value + extra
> +       * additional space used by the hardware */
> +      uint8_t clear_value_state_size;
> +      /* size of the clear color itself - used to copy it to/from a BO */
>        uint8_t clear_value_size;
>        uint8_t clear_value_offset;
>     } ss;
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index a0aee43bd21..0dafe03442d 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -264,7 +264,7 @@ add_aux_state_tracking_buffer(struct anv_image *image,
>     }
>  
>     /* Clear color and fast clear type */
> -   unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
> +   unsigned state_size = device->isl_dev.ss.clear_value_state_size + 4;
>  
>     /* We only need to track compression on CCS_E surfaces. */
>     if (image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E) {
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index 37e63f56aa0..b8c381d2665 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -2566,7 +2566,7 @@ anv_image_get_fast_clear_type_addr(const struct anv_device *device,
>  {
>     struct anv_address addr =
>        anv_image_get_clear_color_addr(device, image, aspect);
> -   addr.offset += device->isl_dev.ss.clear_value_size;
> +   addr.offset += device->isl_dev.ss.clear_value_state_size;
>     return addr;
>  }
>  
> -- 
> 2.14.3
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Mon, Feb 26, 2018 at 01:07:54PM -0800, Jordan Justen wrote:
> On 2018-02-21 13:45:14, Rafael Antognolli 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@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
> > @@ -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"/>
> > +    <!-- Reserved - MBZ -->
> 
> I'm not sure about this comment. Maybe:
> 
>     <!-- DW4-7 required by hardware, but not used by software -->

OK, will update that.

> I was going to ask about gen11.xml, but since isl.c is not compiled
> per gen, I'm not quite sure how things work with structs in the
> genxml.

Hmmm... good point, I just forgot gen11. Yeah, it's not compiled per gen
but isl.c uses genxml/genX_bits.h, that is generated from all xml. I'll
update that.

> > +  </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;
> >     }
> >     assert(RENDER_SURFACE_STATE_SurfaceBaseAddress_start(info) % 8 == 0);
> >     dev->ss.addr_offset =
> > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> > index 209769a9a99..f1b38efed44 100644
> > --- a/src/intel/isl/isl.h
> > +++ b/src/intel/isl/isl.h
> > @@ -963,6 +963,11 @@ struct isl_device {
> >        uint8_t aux_addr_offset;
> >  
> >        /* Rounded up to the nearest dword to simplify GPU memcpy operations. */
> > +
> > +      /* size of the state buffer used to store the clear value + extra
> > +       * additional space used by the hardware */
> > +      uint8_t clear_value_state_size;
> > +      /* size of the clear color itself - used to copy it to/from a BO */
> >        uint8_t clear_value_size;
> >        uint8_t clear_value_offset;
> >     } ss;
> > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > index a0aee43bd21..0dafe03442d 100644
> > --- a/src/intel/vulkan/anv_image.c
> > +++ b/src/intel/vulkan/anv_image.c
> > @@ -264,7 +264,7 @@ add_aux_state_tracking_buffer(struct anv_image *image,
> >     }
> >  
> >     /* Clear color and fast clear type */
> > -   unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
> > +   unsigned state_size = device->isl_dev.ss.clear_value_state_size + 4;
> >  
> >     /* We only need to track compression on CCS_E surfaces. */
> >     if (image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E) {
> > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> > index 37e63f56aa0..b8c381d2665 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -2566,7 +2566,7 @@ anv_image_get_fast_clear_type_addr(const struct anv_device *device,
> >  {
> >     struct anv_address addr =
> >        anv_image_get_clear_color_addr(device, image, aspect);
> > -   addr.offset += device->isl_dev.ss.clear_value_size;
> > +   addr.offset += device->isl_dev.ss.clear_value_state_size;
> >     return addr;
> >  }
> >  
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wed, Feb 21, 2018 at 1:45 PM, Rafael Antognolli <
rafael.antognolli@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@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 think this means dropping the previous patch entirely and just making
this patch add a size field.


>     }
>     assert(RENDER_SURFACE_STATE_SurfaceBaseAddress_start(info) % 8 == 0);
>     dev->ss.addr_offset =
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index 209769a9a99..f1b38efed44 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -963,6 +963,11 @@ struct isl_device {
>        uint8_t aux_addr_offset;
>
>        /* Rounded up to the nearest dword to simplify GPU memcpy
> operations. */
> +
> +      /* size of the state buffer used to store the clear value + extra
> +       * additional space used by the hardware */
> +      uint8_t clear_value_state_size;
>

Maybe call this clear_color_state_size since it is the size of the
CLEAR_COLOR state.


> +      /* size of the clear color itself - used to copy it to/from a BO */
>        uint8_t clear_value_size;
>        uint8_t clear_value_offset;
>     } ss;
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index a0aee43bd21..0dafe03442d 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -264,7 +264,7 @@ add_aux_state_tracking_buffer(struct anv_image *image,
>     }
>
>     /* Clear color and fast clear type */
> -   unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
> +   unsigned state_size = device->isl_dev.ss.clear_value_state_size + 4;
>
>     /* We only need to track compression on CCS_E surfaces. */
>     if (image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E) {
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private
> .h
> index 37e63f56aa0..b8c381d2665 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -2566,7 +2566,7 @@ anv_image_get_fast_clear_type_addr(const struct
> anv_device *device,
>  {
>     struct anv_address addr =
>        anv_image_get_clear_color_addr(device, image, aspect);
> -   addr.offset += device->isl_dev.ss.clear_value_size;
> +   addr.offset += device->isl_dev.ss.clear_value_state_size;
>     return addr;
>  }
>
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
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@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@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 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 =
>     diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
>     index 209769a9a99..f1b38efed44 100644
>     --- a/src/intel/isl/isl.h
>     +++ b/src/intel/isl/isl.h
>     @@ -963,6 +963,11 @@ struct isl_device {
>            uint8_t aux_addr_offset;
> 
>            /* Rounded up to the nearest dword to simplify GPU memcpy
>     operations. */
>     +
>     +      /* size of the state buffer used to store the clear value + extra
>     +       * additional space used by the hardware */
>     +      uint8_t clear_value_state_size;
> 
> 
> Maybe call this clear_color_state_size since it is the size of the CLEAR_COLOR
> state.

Ack.

> 
>     +      /* size of the clear color itself - used to copy it to/from a BO */
>            uint8_t clear_value_size;
>            uint8_t clear_value_offset;
>         } ss;
>     diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
>     index a0aee43bd21..0dafe03442d 100644
>     --- a/src/intel/vulkan/anv_image.c
>     +++ b/src/intel/vulkan/anv_image.c
>     @@ -264,7 +264,7 @@ add_aux_state_tracking_buffer(struct anv_image *image,
>         }
> 
>         /* Clear color and fast clear type */
>     -   unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
>     +   unsigned state_size = device->isl_dev.ss.clear_value_state_size + 4;
> 
>         /* We only need to track compression on CCS_E surfaces. */
>         if (image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E) {
>     diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private
>     .h
>     index 37e63f56aa0..b8c381d2665 100644
>     --- a/src/intel/vulkan/anv_private.h
>     +++ b/src/intel/vulkan/anv_private.h
>     @@ -2566,7 +2566,7 @@ anv_image_get_fast_clear_type_addr(const struct
>     anv_device *device,
>      {
>         struct anv_address addr =
>            anv_image_get_clear_color_addr(device, image, aspect);
>     -   addr.offset += device->isl_dev.ss.clear_value_size;
>     +   addr.offset += device->isl_dev.ss.clear_value_state_size;
>         return addr;
>      }
>    
>     --
>     2.14.3
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev@lists.freedesktop.org
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>
On Tue, Feb 27, 2018 at 9:35 AM, Rafael Antognolli <
rafael.antognolli@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@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@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?


> > 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 =
> >     diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> >     index 209769a9a99..f1b38efed44 100644
> >     --- a/src/intel/isl/isl.h
> >     +++ b/src/intel/isl/isl.h
> >     @@ -963,6 +963,11 @@ struct isl_device {
> >            uint8_t aux_addr_offset;
> >
> >            /* Rounded up to the nearest dword to simplify GPU memcpy
> >     operations. */
> >     +
> >     +      /* size of the state buffer used to store the clear value +
> extra
> >     +       * additional space used by the hardware */
> >     +      uint8_t clear_value_state_size;
> >
> >
> > Maybe call this clear_color_state_size since it is the size of the
> CLEAR_COLOR
> > state.
>
> Ack.
>
> >
> >     +      /* size of the clear color itself - used to copy it to/from a
> BO */
> >            uint8_t clear_value_size;
> >            uint8_t clear_value_offset;
> >         } ss;
> >     diff --git a/src/intel/vulkan/anv_image.c
> b/src/intel/vulkan/anv_image.c
> >     index a0aee43bd21..0dafe03442d 100644
> >     --- a/src/intel/vulkan/anv_image.c
> >     +++ b/src/intel/vulkan/anv_image.c
> >     @@ -264,7 +264,7 @@ add_aux_state_tracking_buffer(struct anv_image
> *image,
> >         }
> >
> >         /* Clear color and fast clear type */
> >     -   unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
> >     +   unsigned state_size = device->isl_dev.ss.clear_value_state_size
> + 4;
> >
> >         /* We only need to track compression on CCS_E surfaces. */
> >         if (image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E) {
> >     diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private
> >     .h
> >     index 37e63f56aa0..b8c381d2665 100644
> >     --- a/src/intel/vulkan/anv_private.h
> >     +++ b/src/intel/vulkan/anv_private.h
> >     @@ -2566,7 +2566,7 @@ anv_image_get_fast_clear_type_addr(const
> struct
> >     anv_device *device,
> >      {
> >         struct anv_address addr =
> >            anv_image_get_clear_color_addr(device, image, aspect);
> >     -   addr.offset += device->isl_dev.ss.clear_value_size;
> >     +   addr.offset += device->isl_dev.ss.clear_value_state_size;
> >         return addr;
> >      }
> >
> >     --
> >     2.14.3
> >
> >     _______________________________________________
> >     mesa-dev mailing list
> >     mesa-dev@lists.freedesktop.org
> >     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
>
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@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@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@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 =
On Tue, Feb 27, 2018 at 11:55 AM, Rafael Antognolli <
rafael.antognolli@intel.com> wrote:

> 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@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@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@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 see what you mean.  I'm not quite sure what the right solution is without
knowing exactly what piece of code you're looking at.  The only one I can
think of is init_fast_clear_color and I think the right thing there might
be to just structure the gen split a little differently and do

if (GEN_GEN <= 8) {
   /* Fill out the one dword */
} else {
   for (unsigned i = 0; i < 4; i++) {
      /* Fill out the dwords */
   }
}


> 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 =
>